opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

Remotetap extension concept

Open wildum opened this issue 1 year ago • 11 comments

Description

This is the core part of the remote tap concept described here: https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/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: https://github.com/open-telemetry/opentelemetry-collector/pull/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.

wildum avatar Aug 26 '24 13:08 wildum

Please create this PR in contrib? We will not have new components in this repository right now.

atoulme avatar Aug 26 '24 22:08 atoulme

Ah okay, then I will move the component back to contrib and will only update the processorhelper logic in core

wildum avatar Aug 27 '24 09:08 wildum

@atoulme I moved the component from this PR to contrib: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/34872. Only the changes related to the framework changes are left in this PR

wildum avatar Aug 27 '24 13:08 wildum

@djaglowski I made the changes, let me know what you think :)

wildum avatar Sep 17 '24 12:09 wildum

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

wildum avatar Sep 19 '24 14:09 wildum

Unfortunately I think we have the same stability concern with component. Basically, anything mentioned here as part of the roadmap is not going to work for something so new. Maybe @open-telemetry/collector-approvers have an idea.

djaglowski avatar Sep 19 '24 20:09 djaglowski

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 avatar Sep 25 '24 15:09 wildum

@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 avatar Sep 30 '24 21:09 djaglowski

@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)?

wildum avatar Oct 01 '24 09:10 wildum

The bad news is that it's not great timing with the 1.0 work

I believe the changes are isolated to the service package, which is not part of the 1.0 effort, if I understand correctly. :)

djaglowski avatar Oct 01 '24 12:10 djaglowski

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Oct 16 '24 03:10 github-actions[bot]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions[bot] avatar Oct 31 '24 03:10 github-actions[bot]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

github-actions[bot] avatar Nov 16 '24 03:11 github-actions[bot]