Skip to content
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] Status Aggregation in Core #10058

Closed
mwear opened this issue Apr 30, 2024 · 4 comments · Fixed by #10958
Closed

[component] Status Aggregation in Core #10058

mwear opened this issue Apr 30, 2024 · 4 comments · Fixed by #10958

Comments

@mwear
Copy link
Member

mwear commented Apr 30, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

mwear commented May 2, 2024

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.

codeboten pushed a commit that referenced this issue Aug 22, 2024
#### 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:
- #9823
- #10058
- #9957
- #9324
- #6506

---------

Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
@TylerHelmuth
Copy link
Member

With #10413 merged, I am removing this from the component 1.0 milestone and Collector V1 project.

sfc-gh-sili pushed a commit to sfc-gh-sili/opentelemetry-collector that referenced this issue Aug 23, 2024
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:
- open-telemetry#9823
- open-telemetry#10058
- open-telemetry#9957
- open-telemetry#9324
- open-telemetry#6506

---------

Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
github-merge-queue bot pushed a commit that referenced this issue Dec 17, 2024
… to Stopping (#10958)

#### Description
In #10058 I mentioned:

> 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.

This PR makes the suggested change and updates the documentation to
reflect that. As this is an internal change, I have not included a
changelog. Also note, we can close #10058 after this as we've already
removed status aggregation from core during the recent component status
refactor.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes #10058

<!--Describe what testing was performed and which tests were added.-->
#### Testing
units

<!--Describe the documentation added.-->
#### Documentation
Updated docs/component-status.md and associated diagram.

<!--Please delete paragraphs that you did not use before submitting.-->

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Antoine Toulme <atoulme@splunk.com>
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this issue Dec 19, 2024
… to Stopping (open-telemetry#10958)

#### Description
In open-telemetry#10058 I mentioned:

> 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.

This PR makes the suggested change and updates the documentation to
reflect that. As this is an internal change, I have not included a
changelog. Also note, we can close open-telemetry#10058 after this as we've already
removed status aggregation from core during the recent component status
refactor.

<!-- Issue number if applicable -->
#### Link to tracking issue
Fixes open-telemetry#10058

<!--Describe what testing was performed and which tests were added.-->
#### Testing
units

<!--Describe the documentation added.-->
#### Documentation
Updated docs/component-status.md and associated diagram.

<!--Please delete paragraphs that you did not use before submitting.-->

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Antoine Toulme <atoulme@splunk.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants