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

[service/internal] Allow components to transition from PermanentError to Stopping #10958

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

mwear
Copy link
Member

@mwear mwear commented Aug 23, 2024

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.

Link to tracking issue

Fixes #10058

Testing

units

Documentation

Updated docs/component-status.md and associated diagram.

@mwear mwear requested review from a team and songy23 August 23, 2024 17:11
@mwear mwear force-pushed the allow-permanent-to-stopping branch from 3c75b4c to 29263d4 Compare August 23, 2024 17:29
Copy link

codecov bot commented Aug 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.43%. Comparing base (4782ad0) to head (a965a63).
Report is 80 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10958   +/-   ##
=======================================
  Coverage   91.43%   91.43%           
=======================================
  Files         447      447           
  Lines       23743    23745    +2     
=======================================
+ Hits        21710    21712    +2     
  Misses       1657     1657           
  Partials      376      376           

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

@songy23 songy23 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 26, 2024
@songy23 songy23 requested a review from TylerHelmuth August 26, 2024 14:19
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Sep 10, 2024
@mwear
Copy link
Member Author

mwear commented Sep 10, 2024

Pinging since the stalebot is engaging.

@github-actions github-actions bot removed the Stale label Sep 11, 2024
@mwear mwear force-pushed the allow-permanent-to-stopping branch from 29263d4 to 2bf1a58 Compare October 2, 2024 22:43
@mwear mwear requested a review from a team as a code owner October 2, 2024 22:43
Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Oct 19, 2024
Copy link
Contributor

github-actions bot commented Nov 6, 2024

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

@github-actions github-actions bot closed this Nov 6, 2024
@mwear
Copy link
Member Author

mwear commented Nov 20, 2024

Is there any chance this can be reopened? It was approved, but never merged.

@TylerHelmuth TylerHelmuth reopened this Nov 20, 2024
@TylerHelmuth TylerHelmuth added ready-to-merge Code review completed; ready to merge by maintainers and removed Stale labels Nov 20, 2024
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that this didn't get attention. Will merge this on Monday unless there are comments before then

@mx-psi mx-psi added this pull request to the merge queue Dec 17, 2024
Merged via the queue into open-telemetry:main with commit 58a5ffc Dec 17, 2024
50 checks passed
@github-actions github-actions bot added this to the next release milestone Dec 17, 2024
HongChenTW pushed a commit to HongChenTW/opentelemetry-collector that referenced this pull request 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 <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[component] Status Aggregation in Core
5 participants