Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remotetap extension concept #10963

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wildum
Copy link
Contributor

@wildum wildum commented Aug 26, 2024

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.

@atoulme
Copy link
Contributor

atoulme commented Aug 26, 2024

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

@wildum
Copy link
Contributor Author

wildum commented Aug 27, 2024

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

@wildum
Copy link
Contributor Author

wildum commented Aug 27, 2024

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

service/internal/graph/graph.go Outdated Show resolved Hide resolved
processor/processorhelper/traces.go Outdated Show resolved Hide resolved
@wildum
Copy link
Contributor Author

wildum commented Sep 17, 2024

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

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@wildum wildum requested a review from a team as a code owner September 19, 2024 14:21
@wildum
Copy link
Contributor Author

wildum commented Sep 19, 2024

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

@djaglowski
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants