[component] Component status reporting rfc

[TylerHelmuth]

Description

Adds an RFC for component status reporting. The main goal is to define what component status reporting is, our current, implementation, and how such a system interacts with a 1.0 component. When merged, the following issues will be unblocked:

• Confused about the difference between PermanentError vs FatalError #9823
• [component] Status Aggregation in Core #10058
• [component] Runtime status reporting #9957
• [component] component.go - when is component.Start returning errors? #9324
• Review component public API #6506

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.35%. Comparing base (3ffb41e) to head (d56bf56).

❗ Current head d56bf56 differs from pull request most recent head ca4fd66

Please upload reports for the commit ca4fd66 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10413      +/-   ##
==========================================
- Coverage   92.35%   92.35%   -0.01%     
==========================================
  Files         386      386              
  Lines       18408    18370      -38     
==========================================
- Hits        17001    16965      -36     
+ Misses       1053     1052       -1     
+ Partials      354      353       -1     

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

[atoulme]

Let's not do that please.

[TylerHelmuth]

@atoulme is there a specific objection to Host.ReportFatalError?

[mwear]

If we keep the behavior of having the collector shutdown when it experiences a FatalError, I think we will need to reinstate Host.ReportFatalError to comply with goal 5:

5. Component health reporting must only be a mechanism for reporting health - it should have no mechanisms for taking actions on the health it reports. How consumers of the health reporting system respond to component updates is not a concern of the health reporting system.

I would not be opposed to bringing back Host.ReportFatalError. If we did we can update the automation to report a FatalError for the component that reported it. It keeps component status reporting out of the FatalError API, but still allows us to generate a fatal error event for Watchers.

[djaglowski]

Host.ReportFatalError seems like a violation of a goal 5 because it is essentially reporting a status AND saying what to do about it.

If we want these to be decoupled, then isn't the solution to just allow components to report fatal/permanent errors w/o any expectations for how that will be handled? Then a separate layer can decide what to do about it. e.g. shut down the collector, attempt to restart components, etc.

[TylerHelmuth]

If we want these to be decoupled, then isn't the solution to just allow components to report fatal/permanent errors w/o any expectations for how that will be handled? Then a separate layer can decide what to do about it. e.g. shut down the collector, attempt to restart components, etc.

If we want to achieve Goal 1 (components are not required to emit status from within their own code), then we'll need some mechanism for a component to tell the Host to shutdown (assuming we choose to keep that feature).

Host.ReportFatalError alone isn't in violation of Goal 5 if it is added back exactly as we had it before. The contract would be "A component can call this function to stop the Collector". The Host would not be taking any action on a Status reported from the Health Reporting System, it would be taking action based on the Host interfaces contract.

Adding back Host.ReportFatalError before 1.0 Component is essentially saying we agree that the component.Host interface is a stable way to interact with the Host, but the Health Reporting System is not stable yet.

[djaglowski]

I would be in favor of not allowing components to directly stop the collector. Instead, I think there should only be able to report a permanent/fatal error (i.e. whatever is the maximum severity error) and then it is always up to the collector as a whole to decide whether that warrants shutting down.

[TylerHelmuth]

I think there should only be able to report a permanent/fatal error (i.e. whatever is the maximum severity error) and then it is always up to the collector as a whole to decide whether that warrants shutting down.

This is what we have today, but it effectively is a direct way to stop the collector. So the feature "I want to stop the collector" is locked behind the Status Reporting system.

[atoulme]

Sounds like the right future proof approach. We can and should have the collector decide of its behavior when an error is reported.

[TylerHelmuth]

If we remove this feature, we'd need to decide how components that are currently doing

if errGrpc := jr.grpc.Serve(ln); !errors.Is(errGrpc, grpc.ErrServerStopped) && errGrpc != nil {
    jr.settings.ReportStatus(component.NewFatalErrorEvent(errGrpc))
}

Would handle the error returned from .Serve.

One option is for the component to return a NewPermanent error, but according to goal 1, component status reporting must be optional. So for components that are not opt-in to component status reporting, and no longer have Host.ReportFatalError, what would they do?

[djaglowski]

One option is for the component to return a NewPermanent error, but according to goal 1, component status reporting must be optional.

I don't understand the point of goal 1. Why isn't it acceptable to expect them report a permanent error?

[TylerHelmuth]

Why isn't it acceptable to expect them report a permanent error?

At the moment I am working under the assumption that our current Status Reporting System is too new to be marked stable in component 1.0. Under that assumption, it needs to be ok for a component to NOT be using ReportStatus in order to interact with the Collector Host, which, until Status Reporting was added, was always done via component.Host.

If we want all the custom components out in the wild to be as compatible with component 1.0 as possible, then relying more on component.Host to interact with the Collector Host is better than status reporting.

Maybe there is a future where component.Host isn't needed anymore and components only use status reporting, but I don't think we're there yet. At the moment I view status reporting as a great feature that components SHOULD take advantage of, but are not required to.

[djaglowski]

When you say "interact with the Collector Host", are we only talking about components being able to force the collector to shut down?

[TylerHelmuth]

Yes, but the other ways a component can interact with the host are accessing the available extensions and accessing available factories.

[djaglowski]

It sounds like some notion of reporting a fatal error at any time is the only status-related interaction that we need to pin down for a 1.0 of component. I propose that we commit to such a mechanism for 1.0, but we do not commit to a specific behavior in reaction to the event.

This leaves us room to offer multiple strategies for reacting to a such an event, including:

• Immediate shutdown of the collector.
• Some automatic attempt to restart the component and/or component graph.
• Custom reactions directed via opamp.
• A full blown component status system which includes the notion of a fatal/permanent error.

[mx-psi]

My gut feeling is that ReportStatus should be either part of component.Host or an optional interface to be implemented by component.Hosts

[TylerHelmuth]

I like the idea of an optional interface that component.Hosts can implement.

[mwear]

The larger goal is for components to report runtime status, see #9957. One issue with implementing ReportStatus on host, is that it's passed into start, and for a component to have access to ReportStatus outside of Start it needs to save a handle, either to the function, or to the host. Few do this, but most components save a handle to TelemetrySettings, which provides similar mechanisms for the component to report data about itself. It's not impossible for it be implemented on Host, but its an inconvenient location.

[mx-psi]

Few do this

I think this is something we can remediate with good examples, documentation and possibly some autogenerated test where we assert common types of status transitions (e.g. failed to create a server).

The only difference I see is that you can't report status during component creation, but I see that as a feature, not a bug (there's going to be no status transition before a component has been initialized).

[mwear]

I am more interested in the ergonomics for component authors. I think its natural for the API to report status to be in the same place as I find a meter, or logger for a component. The extra step of explicitly having to save a separate handle to host, or its ReportStatus method to use outside of Start makes the API more cumbersome to use.

[mx-psi]

The advantages I see if we put this in component.Host are:

1. Allowing component status to evolve independently from the rest of the core APIs and decoupling this effort from Collector 1.0 (the most important benefit in my mind)
2. Making it impossible for components to misuse ReportStatus during creation time
3. Potentially removing the awkwardness in having both a component.TelemetrySettings and servicetelemetry.TelemetrySettings, and simplifying the implementation of Allow overriding telemetry providers #4970

I feel like these outweigh component.TelemetrySettings benefits, but it would be great to have a similar list so we can consider pros/cons

[mwear]

On point 3, we will actually introduce the same awkwardness in a different spot. We'll end up with a servicehost.Host and a component.Host. This transition from host to telemetry settings was made once before during the original implementation. See this commit for how things looked when ReportStatus was on Host: b45540e.

[mx-psi]

Thanks for the context, I think I wasn't super clear on (3). We have several vendors interested in providing their own meter provider, tracer provider, logger... (see #4970) so moving this awkwardness to component.Host would potentially simplify adding support for this: all other fields in component.TelemetrySettings are governed by the service::telemetry config section and we could more easily use a factory pattern if we were left with the other fields. That said, I don't think (3) should be the deciding factor, I see (1) as the most important one.

[mwear]

We would also need to confirm that we can still support sharedcomponent with moving the API to host. The sharedcomponent requirements were not fully understood until sometime after b45540e. The requirements I'm referring to are discussed here: #10059 (comment). They are also outlined in the new document: https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/component-status.md#implementation-details.

[mwear]

My first impression is that supporting sharedcomponent will be a challenge if we move ReportStatus from TelemetrySettings to Host. We were able to solve this problem from within sharedcomponent by chaining together the ReportStatus functions for each instance the component represents. This is done through the TelemetrySettings instances passed to LoadOrStore which is called during create. It's important that we have a ReportStatus function that can report for all instances of a sharecomponent before start-time, because the component needs to report the starting events for its instances at the same time. Trying to discover the instances from within the sharedcomponent at start-time will not work because its already too late. We would need a way to know what instances a sharedcomponent represents from "outside" the sharedcomponent to make this work. That said, the current sharedcomponent situation is not ideal and perhaps needs improvement anyways.

[djaglowski]

When designing the existing component status system, we faced many challenges related to component instances. For example, a single processor configuration may be used in multiple pipelines, in which case multiple instances of the processor will be instantiated. Another example, a receiver or exporter which is used in pipelines of different data types will have one instance per data type. Should these instances share a single status or do they each have their own? If each instance has its own status, how do we uniquely identify these instances and communicate each one's status to users?

In my opinion this is a problem which needs to be addressed in any status system design. Alternatively, I think we should consider ways to remove these problems by making changes to the way components are instanced such that all instances correspond to exactly one configuration in the collector config.

[djaglowski]

I think having a full opt-in system will create a lot of unnecessary disparity between components. The collector controls the basic lifecycle of components, so why wouldn't it report this?

Why wouldn't the collector automatically report the basic lifecycle of all components (starting -> running -> stopping -> stopped) and allow each component to opt-in to additional detail (recoverable error -> recovered).

[TylerHelmuth]

This particular goal is supposed to be scoped just to components. The intention is to allow components to be usable in a collector without utilizing any health reporting status framework.

Allowing the Collector Host to report the basic lifecycle checks it can handle is handle via Goal 4.

Should this first goal be reworded to make it clearer that this is only about components?

[djaglowski]

Yeah I think it could be clearer. I think the question of whether or not we will force implementations of component.Host to implement the basic lifecycle should probably be answered first, since the language in this goal depends on whether or not that can be assumed.

[djaglowski]

What is the Collector Host?

[TylerHelmuth]

In this case it is the Collector Service (I think technically it is serviceHost on serivce.Service, which is implementing component.Host.

[djaglowski]

Is there a more technical way we could word this, maybe just say component.Host?

[mx-psi]

Maybe "service's component.Host implementation"?

[TylerHelmuth]

I attempted to clarify the term on line 13:

For the sake of simplicity, when `Host` is referenced in this document you can think of the Collector service, which manages all the collector pipelines and the components within.

But we can use service.Service instead.

[djaglowski]

Host.ReportFatalError seems like a violation of a goal 5 because it is essentially reporting a status AND saying what to do about it.

If we want these to be decoupled, then isn't the solution to just allow components to report fatal/permanent errors w/o any expectations for how that will be handled? Then a separate layer can decide what to do about it. e.g. shut down the collector, attempt to restart components, etc.

[djaglowski]

One option is for the component to return a NewPermanent error, but according to goal 1, component status reporting must be optional.

I don't understand the point of goal 1. Why isn't it acceptable to expect them report a permanent error?

[djaglowski]

Suggested change

	### No way to identify components that are not reporting status
	Goal 1 states that consumers of component status reporting must be able to identify components in use that have not opted in to component status reporting. Our current implementation does not have this feature.

All components should have their basic lifecycle reported automatically. Whether the component itself reports additional granularity about it's state may be optional, but I don't see why it's important.

[TylerHelmuth]

All components should have their basic lifecycle reported automatically

Agreed, thats handled by Goal 4. Goal 1 is attempting to claim that a component should not be required to call settings.ReportStatus in order to be called a component and to function successfully in a collector build.