-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[component] Remove ReportStatus from component.TelemetrySettings #10777
[component] Remove ReportStatus from component.TelemetrySettings #10777
Conversation
cdea2a2
to
2ebc539
Compare
2ebc539
to
6765387
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I have one non-actionable comment and a suggestion for a followup.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10777 +/- ##
==========================================
- Coverage 91.63% 91.60% -0.03%
==========================================
Files 405 404 -1
Lines 19034 18980 -54
==========================================
- Hits 17442 17387 -55
- Misses 1232 1234 +2
+ Partials 360 359 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocked based on discussion on the 2024-07-08 Collector stability WG meeting until after v0.107.0 is released
I recognize that this PR is still really big and many won't have an appetite for reviewing it. I am gonna try to break it up even more: |
#### Description Adds a `Reporter` interface that represents how a `component.Host` implementation could expose the ability to report a status. You can see how this interface will be used by looking at Related to #10777 <!-- Issue number if applicable --> #### Link to tracking issue Related to #10777 Related to #10413 <!--Describe what testing was performed and which tests were added.--> #### Testing Added unit test
#### Description This is a prep PR to reduce the size of #10777. As part of the work to make our `component.Host` implementation implement `componentstatus.Reporter` (see #10852), the `host` struct and `graph` logic need to be closer together. This is because, as part of #10777 `StartAll` is changed to depend on our specific `Host` type instead of a `component.Host`. Our host already has a dependency on `graph`, so it can't be moved into its own module. <!-- Issue number if applicable --> #### Link to tracking issue Related to #10777 Related to #10413
@TylerHelmuth Can you fix the merge conflicts? |
@TylerHelmuth How do you plan on handling contrib tests failure? Is there any way to make them pass before merging this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. As discussed on the 2024-08-14 Collector Stability WG meeting I will merge this by EOW if there are no further comments
@mwear and I will handle the breaking changes via a We'll also end up duplicating the logic from core's |
I'll handle healthcheckv2extension. We can divide up anything else that remains. |
Description
This PR removes
ReportStatus
fromcomponent.TelemetrySettings
and instead expects components to check if theircomponent.Host
implements a newcomponentstatus.Reporter
interface.Link to tracking issue
Related to #10725
Related to #10413
Testing
unit tests and a sharedinstance e2e test.
The contrib tests will fail because this is a breaking change. If we merge this I and @mwear can commit to updating contrib before the next release.