[component] Move component status features to its own module #10725

TylerHelmuth · 2024-07-25T18:37:52Z

Description

This PR is an attempt are removing component status reporting features from the component module. The technique was to make a new componentstatus.StatusReporter interface that component.Host could optionally implement. This has impacts:

We no longer need a servicetelemetry.TelemetrySettings struct.
Components no longer get access to a ReportStatus function until the component.Start method provides the host.
Components that use sharedcomponent will now only report a status for instances that have been started when an error in start occurs. As an example if an otlpreceiver is used in a traces, metrics, and logs pipeline, when the host make the initial call to Start, say for the traces pipeline, sharedcomponent will only report the error returned from Start for the traces instance, and then continue on to shutdown.

If this solution's general idea is tenable, then I believe it could be broken down into slightly smaller PRs, such as:

Remove the need for servicetelemetry.TelemetrySettings: [service] Remove servicetelemetry.TelemetrySettings #10728
Move status stuff into its own module: [component] Move component status reporting public API to new componentstatus module #10730
Remove ReportStatus from TelemetrySettings and implement the StatusReporter interface: [component] Remove ReportStatus from component.TelemetrySettings #10777

Link to tracking issue

Related to #10413

codecov · 2024-07-25T19:15:18Z

Codecov Report

Attention: Patch coverage is 81.27660% with 44 lines in your changes missing coverage. Please review.

Project coverage is 92.12%. Comparing base (95902c1) to head (900cebe).
Report is 1 commits behind head on main.

Files	Patch %	Lines
service/internal/graph/graph.go	77.41%	21 Missing ⚠️
service/service.go	69.56%	0 Missing and 7 partials ⚠️
receiver/otlpreceiver/otlp.go	0.00%	6 Missing ⚠️
internal/sharedcomponent/sharedcomponent.go	87.87%	4 Missing ⚠️
extension/zpagesextension/zpagesextension.go	0.00%	3 Missing ⚠️
processor/processortest/unhealthy_processor.go	50.00%	1 Missing and 1 partial ⚠️
service/internal/status/status.go	97.05%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10725      +/-   ##
==========================================
- Coverage   92.18%   92.12%   -0.06%     
==========================================
  Files         403      400       -3     
  Lines       18792    18816      +24     
==========================================
+ Hits        17323    17335      +12     
- Misses       1109     1121      +12     
  Partials      360      360

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TylerHelmuth · 2024-07-25T19:59:28Z

internal/sharedcomponent/sharedcomponent.go

+	if c.hostWrapper == nil {
+		c.hostWrapper = &hostWrapper{
+			host:           host,
+			sources:        make([]componentstatus.StatusReporter, 0),
+			previousEvents: make([]*componentstatus.StatusEvent, 0),
+		}
+	}
+
+	statusReporter, isStatusReporter := host.(componentstatus.StatusReporter)
+	if isStatusReporter {
+		c.hostWrapper.addSource(statusReporter)
+	}


The new technique for sharedcomponent is to build the chain or reporting sources as Start is getting called. Previously reported events are remembered so that if Start is called multiple times, each subsequent StatusReporter and get the previously emitted StatusEvents.

mx-psi

This approach seems reasonable to me, I think the ergonomics are acceptable and it allows us to separate the status reporting mechanism from the rest of the component module

extension/extension.go

receiver/otlpreceiver/otlp.go

djaglowski

The approach looks good to me overall.

component/componentstatus/status.go

internal/sharedcomponent/sharedcomponent.go

djaglowski · 2024-07-26T15:47:45Z

One other note - component.InstanceID was added for the sake of component status (though in theory it may be useful otherwise). I think we may want to move it out of component as well.

TylerHelmuth · 2024-07-26T16:36:33Z

@djaglowski I've moved InstanceID to componentstatus (which in the actually implementation I'll handle by adding it when the content is moved to its own package and deprecating the one in component).

@mx-psi the fact that this move is possible makes me think maybe #10222 is back on the table, but I need to think about it more.

service/internal/graph/graph.go

mwear

The sharedcomponent changes worry me a little bit. I'd like to see if I can test drive it with the health check extension to make sure it is still working as intended. Otherwise, this generally looks good to me.

TylerHelmuth · 2024-07-26T21:01:22Z

@mwear the key change happens when a component.Start returns a synchronous error, which gets back to the host and ends up terminating the startup process. The other signals for the sharedcomponent won't receive the NewPermanent error, but also they never receiver a Starting status either. They get not statuses reported for them, like another component further down the line that also did not get started.

mwear · 2024-07-26T21:18:15Z

What about the recoverable error case? When reporting status from within the component (I guess this is via host now), it should report status for all of the components it represents. All components represented by the shared component must be in the Starting state before the API can be from called from "within" the shared component. If you report RecoverableError during start, but some instances are not Starting, you will get an invalid state transition for the unstarted instances, and those instances will be stuck reporting an invalid status. I'm not sure if this makes sense, but this is my concern with this change. It's a little hard to test this end to end with the health check extension to verify because there are so many changes. In any case, if these words make any sense, and are accurate, we may have to handle this another way.

mwear · 2024-07-26T21:24:34Z

It's possible that the way you are handling previous events solves my concerns. I'd still like to find a way to verify this end to end.

#### Description Reorganizes service to not require `servicetelemetry.TelemetrySettings` and instead depend directly on `component.TelemetrySettings` Whether or not we move forward with #10725 I think this is a useful change for service. #### Testing Unit tests

TylerHelmuth · 2024-07-29T14:08:59Z

@mwear I'll see if I can add a test case in this PR with that example.

TylerHelmuth · 2024-07-29T17:55:17Z

@mwear I've added a very rough e2e test in internal/e2e/status_test.go that tests that a component that is using sharedcomponent does end up eventually reporting all statuses for each instance and that an extension watching for statuses does receive all the events.

This gives me confidence that the statuses do end up in the extensions correctly. How the extension handles the statuses is up to the extension right?

One thing of note is that the existing (in main) graph/sharedinstance implementation runs into an invalid state in the graph's fsm because the graph reports StatusStarting for the current pipeline and the sharedinstance implementation also reports StatusStarting for all instances, one of which was already reported as StatusStarting.

I highlight this as a datapoint that shows that status reporting still needs lots of maturing, and while this PR changes a lot about how status reporting works, I think that is ok since it is still so young. I feel confident that if we do find issues with this implementation later on that we'll be able to make fixes via graph/sharedinstance/componentstatus without needing to break user APIs or component behavior.

mwear · 2024-07-29T20:53:59Z

Thanks for the test @TylerHelmuth, this gives me more confidence. I'm aware of the repeat status reporting that was part of the original implementation. The finite state machine is meant to protect and enforce the lifecycle of a component, and invalid state transitions are not necessarily errors, in many cases should be considered a no-op. I know there is some logging around this that I would be in favor or removing because it gives the impression something unexpected happened, when that's not exactly the case. This is discussed in the status reporting documentation under the runtime section.

During runtime a component should not have to keep track of its state. A component should report status as operations succeed or fail and the finite state machine will handle the rest. Changes in status will result in new status events being emitted. Repeat reports of the same status will no-op. Similarly, attempts to make an invalid state transition, such as PermanentError to OK, will have no effect.

TylerHelmuth · 2024-07-29T22:21:21Z

@mwear do you feel comfortable enough to move forward with this approach via #10730?

mwear · 2024-07-29T22:41:48Z

My biggest reservation is that we need the collector to be observable for it to be 1.0. Part of this means having a functioning health check extension. The current replacement requires component status, so I don't think we can call the collector 1.0 without having component status enabled by default. If we are moving this code for organization purposes, that's fine, but I don't think we can skip this work for 1.0 altogether.

mx-psi · 2024-07-30T09:33:45Z

Part of this means having a functioning health check extension

The healthcheck extension is not in our GA roadmap. I think it's important that we keep working on it, but our current plans don't include this for 1.0 as defined in the GA roadmap

…ntstatus module (#10730) #### Description Duplicates component status reporting features from `component` into a separate module, `componentstatus`. In a future PR, when `component.TelemetrySettings.ReportStatus` is removed, I'll update Core to depend on `componentstatus`. This work isolates component status public API from `component` and `extensions`, which will allow us to move forward with their 1.0 work while component status reporting matures.  #### Link to tracking issue Related to #10725 --------- Co-authored-by: Matthew Wear <matthew.wear@gmail.com>

TylerHelmuth · 2024-07-31T21:06:56Z

Ive created #10777 to complete the transition in core.

github-actions · 2024-08-15T03:16:07Z

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

@mwear

) #### Description This PR removes `ReportStatus` from `component.TelemetrySettings` and instead expects components to check if their `component.Host` implements a new `componentstatus.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. --------- Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>

github-actions · 2024-08-29T03:16:14Z

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

TylerHelmuth force-pushed the component-remove-reporting-from-struct branch 3 times, most recently from 2ea4302 to 6f486c7 Compare July 25, 2024 19:05

Try removing ReportStatus from TelemetrySettings

900cebe

TylerHelmuth force-pushed the component-remove-reporting-from-struct branch from 6f486c7 to 900cebe Compare July 25, 2024 19:15

TylerHelmuth requested review from mwear, djaglowski and mx-psi July 25, 2024 19:57

TylerHelmuth commented Jul 25, 2024

View reviewed changes

This was referenced Jul 25, 2024

[component] Component status reporting rfc #10413

Merged

[service] Remove servicetelemetry.TelemetrySettings #10728

Merged

mx-psi reviewed Jul 26, 2024

View reviewed changes

extension/extension.go Show resolved Hide resolved

receiver/otlpreceiver/otlp.go Outdated Show resolved Hide resolved

djaglowski reviewed Jul 26, 2024

View reviewed changes

component/componentstatus/status.go Outdated Show resolved Hide resolved

internal/sharedcomponent/sharedcomponent.go Outdated Show resolved Hide resolved

internal/sharedcomponent/sharedcomponent.go Outdated Show resolved Hide resolved

TylerHelmuth added 5 commits July 26, 2024 09:49

Add a user-friendly way to report status

c67718f

Update names

bf3b05b

Remove unnecessary checks

d19683c

Reorganize sharedcomponent Start

163e197

Move InstanceID to componentstatus

16975d4

djaglowski reviewed Jul 26, 2024

View reviewed changes

service/internal/graph/graph.go Show resolved Hide resolved

TylerHelmuth mentioned this pull request Jul 26, 2024

[component] Move component status reporting public API to new componentstatus module #10730

Merged

mwear reviewed Jul 26, 2024

View reviewed changes

Add SharedComponent Recoverable error test

9cf0b13

TylerHelmuth mentioned this pull request Jul 31, 2024

[component] Remove ReportStatus from component.TelemetrySettings #10777

Merged

github-actions bot added the Stale label Aug 15, 2024

github-actions bot closed this Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[component] Move component status features to its own module #10725

[component] Move component status features to its own module #10725

TylerHelmuth commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

TylerHelmuth Jul 25, 2024

mx-psi left a comment

djaglowski left a comment

djaglowski commented Jul 26, 2024

TylerHelmuth commented Jul 26, 2024

mwear left a comment

TylerHelmuth commented Jul 26, 2024

mwear commented Jul 26, 2024

mwear commented Jul 26, 2024

TylerHelmuth commented Jul 29, 2024

TylerHelmuth commented Jul 29, 2024 •

edited

Loading

mwear commented Jul 29, 2024

TylerHelmuth commented Jul 29, 2024

mwear commented Jul 29, 2024

mx-psi commented Jul 30, 2024

TylerHelmuth commented Jul 31, 2024

github-actions bot commented Aug 15, 2024

github-actions bot commented Aug 29, 2024

[component] Move component status features to its own module #10725

[component] Move component status features to its own module #10725

Conversation

TylerHelmuth commented Jul 25, 2024 • edited Loading

Description

Link to tracking issue

codecov bot commented Jul 25, 2024 • edited Loading

Codecov Report

TylerHelmuth Jul 25, 2024

Choose a reason for hiding this comment

mx-psi left a comment

Choose a reason for hiding this comment

djaglowski left a comment

Choose a reason for hiding this comment

djaglowski commented Jul 26, 2024

TylerHelmuth commented Jul 26, 2024

mwear left a comment

Choose a reason for hiding this comment

TylerHelmuth commented Jul 26, 2024

mwear commented Jul 26, 2024

mwear commented Jul 26, 2024

TylerHelmuth commented Jul 29, 2024

TylerHelmuth commented Jul 29, 2024 • edited Loading

mwear commented Jul 29, 2024

TylerHelmuth commented Jul 29, 2024

mwear commented Jul 29, 2024

mx-psi commented Jul 30, 2024

TylerHelmuth commented Jul 31, 2024

github-actions bot commented Aug 15, 2024

github-actions bot commented Aug 29, 2024

TylerHelmuth commented Jul 25, 2024 •

edited

Loading

codecov bot commented Jul 25, 2024 •

edited

Loading

TylerHelmuth commented Jul 29, 2024 •

edited

Loading