open-telemetry / opentelemetry-collector

OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
4.3k stars 1.42k forks source link

[component] Status Aggregation in Core #10058

Open mwear opened 4 months ago

mwear commented 4 months ago

Functions to handle status aggregation were added to core when status reporting was originally implemented. These functions were initially adequate while I was working on an alternative health check extension, but as more requirements surfaced, I had to reimplement alternative logic to handle all of the use cases. This PR specifically introduces the alternative aggregation logic and this PR shows how everything works together.

The goal of status aggregation is to take a collection of status events and combine them into a single event that represents the most relevant status based on the collection as a whole. One assumption we made in the original aggregation logic, was that PermanentErrors should take precedence over RecoverableErrors. While this still the default aggregation used in the health check extension, users have the option to include or ignore permanent and / or recoverable errors independently. If a user ignores PermanentErrors, but includes RecoverableErrors, we want RecoverableErrors to take priority over PermanentErrors during aggregation. In order to handle this is this in the extension, I implemented a function that takes an ErrorPriority and returns a suitable aggregation function.

We can divide component statuses into two categories. Lifecycle statuses (eg Starting, Stopping, Stopped) and runtime statuses (eg RecoverableError, PermanentError, FatalError). In the original aggregation logic, error statuses take precedence over lifecycle statuses. In the health check extension I had to change the aggregation logic to give precedence to lifecycle events in order to distinguish between a collector that is starting up or shutting down (a 503 status) or a collector in an error state (a 500).

There is a tangentially related issue with PermanentErrors and the underlying finite state machine that governs transitions between statuses. Currently, a PermanentError is a final state. That is, once a component enters this state, no further transitions are allowed. In light of the work I did on the alternative health check extension, I believe we should allow a transition from PermanentError to Stopping to consistently prioritize lifecycle events for components. This transition also make sense from a practical perspective. A component in a PermanentError state is one that has been started and is running, although in a likely degraded state. The collector will call shutdown on the component (when the collector is shutting down) and we should allow the status to reflect that.

To summarize, the aggregation logic as it exists in core was not usable for the health check extension and needed to be reimplemented. The health check extension needed the ability to prioritize RecoverableErrors over PermanentErrors in some, but not all cases. It also needed to prioritize lifecycle statuses over runtime statuses. A change that was not made, but should be made, is allowing a component to transition from a PermanentError state to Stopping (for consistency). As we work towards component 1.0, we should decide if the aggregation logic we have in core should be replaced by what was implemented for the health check extension, leave it as is, or remove it completely.

TylerHelmuth commented 4 months ago

The collector will call shutdown on the component (when the collector is shutting down) and we should allow the status to reflect that.

Yes this seems totally valid. As part of manually handling the permanent error a graceful shutdown could occur, and that transition should be recorded.

TylerHelmuth commented 4 months ago

One argument for leaving it in core is to try to help components aggregate statuses in a consistent manner so that the meaning of a Status is consistent between components. But seeing as how the very first component to use statuses instantly needed its own aggregation logic, part of me thinks it is an individual component concern.

mwear commented 4 months ago

Once we are happy with the aggregation for the health check extension, I think we can and should promote it to core, or some other shared library. We may have been over zealous in starting with it in core, but the intent was as you point out, to help aggregate status consistently between consumers.

TylerHelmuth commented 4 weeks ago

With https://github.com/open-telemetry/opentelemetry-collector/pull/10413 merged, I am removing this from the component 1.0 milestone and Collector V1 project.