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

Declare how to share a receiver across multiple signals with factory options

Open atoulme opened this issue 9 months ago • 13 comments

Description

Declare how to share a receiver across multiple signals with factory options.

Right now, we have each component looking to reuse across signals use a library under internal/sharedcomponent, and declare their own map of components. We move this map to the factory and use a factory option to set a consumer of a different signal on the existing receiver.

This removes the need to declare and manage internal/sharedcomponent.

This PR lacks test coverage and is meant to collect feedback on the feasibility.

Testing

WIP

Documentation

API docs added.

atoulme avatar May 01 '24 07:05 atoulme

Codecov Report

Attention: Patch coverage is 29.72973% with 52 lines in your changes missing coverage. Please review.

Project coverage is 91.59%. Comparing base (cad2734) to head (5ae1cc8). Report is 463 commits behind head on main.

Files Patch % Lines
receiver/receiver.go 0.00% 51 Missing and 1 partial :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10059      +/-   ##
==========================================
- Coverage   91.87%   91.59%   -0.28%     
==========================================
  Files         360      359       -1     
  Lines       16725    16722       -3     
==========================================
- Hits        15366    15317      -49     
- Misses       1021     1069      +48     
+ Partials      338      336       -2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 01 '24 07:05 codecov[bot]

https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/32809 is open to fix contrib.

atoulme avatar May 01 '24 23:05 atoulme

There are two behaviors related to status reporting that were implemented in sharedcomponent that need to be preserved in this PR. A shared component represents multiple, logical component instances as a single component. For the purposes of status reporting, the shared component must report status for each of the logical components it represents. To complicate matters sightly, there are two ways to report status for a component. It can be reported from outside the component (via the service status.Reporter) or from within the component (via component.TelemetrySettings). In both cases, a single call to ReportStatus needs to report status for each component instance. For example, if you have an OTLP receiver that is part of three pipelines, one for traces, logs, and metrics, three status events would be emitted for each call to ReportStatus. With that as background, I'll explain the two special cases that sharedcomponent currently handles.

  1. sharedcomponent currently chains together the telemetrysettings.ReportStatus functions for each instance of the component. this is how it emits n status events for n logical instances. This function is called from within the component.
  2. graph does automatic status reporting for components during startup. It calls ReportStatus on the service's status.Reporter. This reports status from outside the component. The graph starts components one by one, however, this is a problem for the sharedcomponent, as it should transition all logical instances to Starting before Start is called. This is handled in the shared component by reporting its status from within, preemptively, via its chained telemetrysettings.ReportStatus method. Shutdown works similarly.

The solutions currently part of sharedcomponent are not ideal, IMO, and it'd be great if we can improve upon them in the redesign. Let me know if there is anything I can do to help.

mwear avatar May 02 '24 23:05 mwear

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

github-actions[bot] avatar May 17 '24 03:05 github-actions[bot]

@mwear can we create an integration test that faithfully checks both scenarios you describe? This way we can make sure this continues to work.

atoulme avatar May 28 '24 20:05 atoulme

@mwear can we create an integration test that faithfully checks both scenarios you describe? This way we can make sure this continues to work.

Sure thing. That's an oversight from the original implementation.

mwear avatar May 28 '24 20:05 mwear

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

github-actions[bot] avatar Jun 12 '24 03:06 github-actions[bot]

@atoulme, sorry for the delay. I looked into this today and the two cases I describe are in fact tested by an existing test case: https://github.com/open-telemetry/opentelemetry-collector/blob/65d59d14011f1b4cfa100aa70d42bb0ea8415c54/internal/sharedcomponent/sharedcomponent_test.go#L182-L273 Let me know if any further explanations are needed.

mwear avatar Jun 13 '24 21:06 mwear

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

github-actions[bot] avatar Jul 05 '24 03:07 github-actions[bot]

@atoulme Does this relate to #10534?

mx-psi avatar Jul 18 '24 11:07 mx-psi

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

github-actions[bot] avatar Aug 02 '24 03:08 github-actions[bot]

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

github-actions[bot] avatar Aug 22 '24 03:08 github-actions[bot]

@atoulme Does this relate to #10534?

No, this was a spike to look at how to move away from internal/sharedcomponent.

atoulme avatar Aug 22 '24 06:08 atoulme

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

github-actions[bot] avatar Sep 11 '24 03:09 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 06 '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 Oct 20 '24 03:10 github-actions[bot]