Remotetap extension concept #10963
Remotetap extension concept #10963
Conversation
|
Please create this PR in contrib? We will not have new components in this repository right now. |
|
Ah okay, then I will move the component back to contrib and will only update the processorhelper logic in core |
wildum
force-pushed
the
remotetap-extension-concept
branch
from
1872d48 to
ad62aab
Compare
|
@atoulme I moved the component from this PR to contrib: open-telemetry/opentelemetry-collector-contrib#34872. |
wildum
force-pushed
the
remotetap-extension-concept
branch
from
0b694d2 to
1033b02
Compare
|
@djaglowski I made the changes, let me know what you think :) |
There was a problem hiding this comment.
This looks pretty good to me. I suspect the most important thing to move this forward is to make sure it does not in any way interfere with the effort to mark modules 1.0, since this feature would necessarily be experimental for some time. If I am not mistaken, this means the changes to pdata are likely problematic. Can we move them elsewhere, maybe to processorhelper?
I could not move it to processorhelper because it causes some cyclic dep problems + ideally in the future I would like this interface by other types of components. I moved it to the component pkg, what do you think? IMO it fits great there, next to the telemetry.go that provides components the ability to report telemetry |
|
Unfortunately I think we have the same stability concern with |
|
Moved the publisher interface to the processor/internal pkg. It's not ideal because in the future it would be nice to expand the functionality to other components but that should be ok for a first step |
|
@wildum I spiked out an implementation for a related need. The idea is to wrap components and capture metrics about the telemetry flowing in and out of components. I believe the same mechanism could be used for other things such as remote tapping, and it would be much more robust and useful. There's a lot that needs to be done including seeing what the maintainers say but I wanted to call this out as it may be a better path forward. |
|
@djaglowski I like your approach :) Helpers should be used to provide optional features for components that require special behaviors (for example "WithStart", "WithCapabilities" and "WithShutdown"). Forcing all components to use the helpers by putting features such as the telemetry or the remote tap in the helpers defeats the purpose of the helpers. Such functionalities should be rooted at a deeper level and make use of the graph structure. I'm happy to close my PR and support yours. The bad news is that it's not great timing with the 1.0 work as you mentioned above but the good news is that afaik the modifications don't introduce breaking changes (as long as we keep the helpers)? |
I believe the changes are isolated to the service package, which is not part of the 1.0 effort, if I understand correctly. :) |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description
This is the core part of the remote tap concept described here: open-telemetry/opentelemetry-collector-contrib#34096
This will be used by the new version of the remotetap extension in contrib.
This new version uses the processorhelper to tap into the processor pipeline. ALL processors using the processorhelper to be created will automatically support this feature.
To see what a full version could look like you can check this POC: #10962
When no remotetap stream is open, the additional work on the processor pipeline is negligible (1 map read / proc consumption if the extension is not enabled, 2 map read / proc consumption if the extension is enabled but no streams are open)
Idealy, in the future, the feature would be available to all kinds of components. Taping into the processors that use the processorhelper pkg is the first easy step in this direction.