Introduce component status reporting #6560

tigrannajaryan · 2022-11-15T22:53:34Z

This is an alternate to #6550

Summary of changes

Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today.
Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus().

TODO after this is merged

healthcheck extension must implement StatusWatcher.
Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib.

Open Questions

StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?

bogdandrutu

I like this better, much simpler change and cleaner design (at least for me).

component/status.go

bogdandrutu · 2022-11-15T22:56:21Z

component/status.go

+// StatusSource component that reports a status about itself.
+// The implementation of this interface must be comparable to be useful as a map key.
+type StatusSource interface {
+	ID() ID
+}


Why do you need this interface at all? Simply pass the ID?

We need a key that uniquely identifies the component. ID is not a unique identification. You may have a processor and a receiver that have the same ID (we don't enforce that receivers and processors don't have the same type string - although in reality they are all unique in core and contrib).

Why it needs to be unique? So that the StatusWatcher can maintain a map[StatusSource]Status.

Alternatively we should enforce uniqueness of ID across all component types.

Interesting point... I understand now the need, let me think a bit more, probably better to have this as a struct if we can define it.

Yet another alternate is to require the Host to track the aggregate component status. In that case StatusWatcher will only be notified of that single aggregate bool status. But in that case we have to hard-code the aggregation logic in the Host and lose flexibility in allowing the extension to decide what to do with individual component statuses.

For the "global component ID", I think we need something like:

package component type GlobalID struct { ID ID Kind Kind PipelineID ID // Not empty only if the Kind is Processor }

This also requires #6540 otherwise you can still have duplicates even with GlobalID.

Good point about that. Now I am more confused after I looked at your implementation https://github.com/open-telemetry/opentelemetry-collector/pull/6560/files#diff-bcb3fc758a54acf0dda21a0d9c79b6cf6e6adcf58d0a282c93be50553523b09dR41 and https://github.com/open-telemetry/opentelemetry-collector/pull/6560/files#diff-19abf8e6f60ec275051d1447d4d97f4c55b449192f2cb946e3e4ea44ff49fcc0R69

At least in the statusReportingComponent is not unique, so my suggestion at least improves that :))

I think &statusReportingComponent{} is unique. We create a new instance for every created component. It is not a value of statusReportingComponent{} struct, we create a pointer to a new struct every time.

Added a test that verifies the uniqueness of StatusSource: https://github.com/tigrannajaryan/opentelemetry-collector/blob/2ac7d388cf548e538601796ec9fff25ae5eb6a78/service/service_test.go#L229

bogdandrutu · 2022-11-15T22:56:50Z

component/status.go

+// StatusWatcher is an extra interface for Extension hosted by the OpenTelemetry
+// Collector that is to be implemented by extensions interested in changes to component
+// status.


Any Component or just extensions?

Only extensions can watch. Any component can be a source.

We can add support for any component to watch but that requires more work and I don't know if we need it.

bogdandrutu · 2022-11-15T22:58:29Z

component/status.go

+// Err returns the error associated with the ComponentEvent.
+func (ev *StatusEvent) Err() error {
+	return ev.err
+}


Can this be a "Description" instead?

Do you mean to rename to func (ev *StatusEvent) Description() error?

func (ev *StatusEvent) Description() string not sure we need error as type, just a Description string. Idea being that Description is a bit more generic, can apply to things like StatusRecovering or other status type.

component/status.go

codecov · 2022-11-15T23:08:38Z

Codecov Report

Base: 91.35% // Head: 90.63% // Decreases project coverage by -0.71% ⚠️

Coverage data is based on head (2ac7d38) compared to base (a8d6280).
Patch coverage: 29.09% of modified lines in pull request are covered.

❗ Current head 2ac7d38 differs from pull request most recent head 444ed5d. Consider uploading reports for the commit 444ed5d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6560      +/-   ##
==========================================
- Coverage   91.35%   90.63%   -0.72%     
==========================================
  Files         241      245       +4     
  Lines       13885    13988     +103     
==========================================
- Hits        12684    12678       -6     
- Misses        956     1070     +114     
+ Partials      245      240       -5

Impacted Files	Coverage Δ
component/componenttest/nop_host.go	`90.00% <0.00%> (-10.00%)`	⬇️
component/componenttest/statuswatcher_extension.go	`0.00% <0.00%> (ø)`
component/componenttest/unhealthy_processor.go	`0.00% <0.00%> (ø)`
component/status.go	`0.00% <0.00%> (ø)`
service/internal/servicehost/nop_host.go	`40.00% <40.00%> (ø)`
service/extensions/extensions.go	`81.13% <64.28%> (-1.85%)`	⬇️
service/internal/pipelines/pipelines.go	`94.70% <85.71%> (-0.42%)`	⬇️
service/collector.go	`76.31% <100.00%> (ø)`
service/host.go	`100.00% <100.00%> (ø)`
service/internal/components/host_wrapper.go	`100.00% <100.00%> (ø)`
... and 8 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

tigrannajaryan · 2022-11-16T00:54:21Z

I like this better, much simpler change and cleaner design (at least for me).

Yeah, I also like this one more.

tigrannajaryan · 2022-11-16T00:54:37Z

@open-telemetry/collector-approvers please take a look at this alternate.

tigrannajaryan · 2022-11-16T21:28:38Z

component/host.go

+	// ReportComponentStatus can be used by a component to communicate its status to the Host.
+	// The Host implementations may broadcast this information to interested parties via
+	// StatusWatcher interface.
+	ReportComponentStatus(event *StatusEvent)


Addition of this method is a breaking change. See failure on Jaeger repo: https://github.com/open-telemetry/opentelemetry-collector/actions/runs/3481700391/jobs/5823123326

@bogdandrutu we have a problem with this.

Adding a method to this interface breaks external codebases (like Jaeger).

I do not see a good way to handle this gracefully. We can add the reporting capability to a different interface and probe for it in the component's, but that does not look very nice to me, e.g.

type ComponentStatusAcceptor interface { ReportComponentStatus(event *StatusEvent) } func (c* MyComponent) Start(ctx context.Context, host component.Host) error { if h, ok:=host.(ComponentStatusAcceptor); ok { h.ReportComponentStatus(...) } }

Another approach would be to add a new optional Start2(ctx context.Context, host component.Host2) to Component2 interface and components can opt in to it, but must continue to support Start(). This looks even uglier.

If we don't want to handle this gracefully then we must coordinate this change with Jaeger.

Any other ideas?

I submitted a contrib PR that should remove the dependency on Jaeger collector, so we can make contrib tests pass, but I believe we will still likely need to run through a breaking change.

I see a few other problems with the current state of this interface:

The set of methods doesn't seem to be complete and potentially can change after we release 1.0, e.g. we might want to introduce getters for receivers and processors, not only exporters and extensions.

Most of the methods start with Get which goes against Go recommendations, maybe we want to change them and remove Get prefix.

I believe we need to take the opportunity and make a bigger refactoring that would allow us to solve all the outlined problems at once. We discussed the problem with @bogdandrutu and came up with the following plan:
Keep only one "core" method ReportComponentStatus the Host interface and move all other "getter" methods to optional interfaces. The optional interfaces for exporter and extension getters can be moved to their own packages (exporter/extension), so we don't need to change returned values from Extension|Exporter toComponent as done in #6553 anymore.

I'll play with it, submit a draft PR, then we can combine it with this PR and try to migrate to the new interfaces as gracefully as feasible.

I'm catching up on this PR. It looks like this is where it stalled out. Some thoughts on how to move forward:

The set of methods doesn't seem to be complete and potentially can change after we release 1.0, e.g. we might want to introduce getters for receivers and processors, not only exporters and extensions.

I'm proposing that we deprecate GetExporters (#7370). I don't believe there are valid reasons to need this function or others like it, such as GetReceivers, GetProcessors, etc.

Most of the methods start with Get which goes against Go recommendations, maybe we want to change them and remove Get prefix.

Between renaming GetExtensions to Extensions, deprecating ReportFatalError and GetExporters, and adding ReportComponentStatus, it looks like we want quite a different interface. Hard to believe we will just live with this as is, so ideally we just need to identify the best migration option. It looks like we no longer need to coordinate with Jaeger but presumably this will affect other components defined elsewhere.

I also tried to identify a graceful option. The best I came up with is quite ugly, but would require no unannounced breaking changes and does not require changes to any other interfaces (i.e. component.Component):

Add an optional interface so it is possible to use the new functions. I'll call it component.Host2.

Deprecate ReportFatalError, GetExporters, and GetExtensions to alert users of these specific functions. Recommend the ugly solution of asserting the optional interfaces (temporarily).

After some time Deprecate component.Host to alert all all affected. However, clearly indicate that this interface will be changed to component.Host, rather than removed.

Change component.Host to match component.Host2. Undeprecate component.Host and deprecate component.Host2

This is an alternate to open-telemetry#6550 ## Summary of changes - Add component status concept. Components can report their status via Host.ReportComponentStatus(). Interested extensions can watch status events if they implement StatusWatcher interface. This is similar to how PipelineWatcher works today. - Deprecate Host.ReportFatalError() in favour of Host.ReportComponentStatus(). ## TODO after this is merged - healthcheck extension must implement StatusWatcher. - Replace all ReportFatalError() calls by ReportComponentStatus() calls in core and contrib. ## Open Questions - StatusWatchers need to be able to tell if all current components are healthy. It is assumed that the listeners need to maintain a map of components and track the status of each component. This works only if we assume that the set of components cannot change during the lifetime of the listener. This assumption is true today but can change in the future if we introduce partial pipeline restarts where only modified/added/removed components are recreated (this will break listener's assumption and the map will become invalid). Should we instead keep track of this entire status map in the Host and broadcast the entire status to the listeners as a whole instead of (or in addition to) individual component events?

github-actions · 2022-12-06T03:15:52Z