opentelemetry-collector
opentelemetry-collector copied to clipboard
Synchronized Run and Shutdown for otelcol/Collector
Description:
Adding some synchronization for Run and Shutdown commands in otelcol/collector.go
Should deal with all 3 requirements from the issue:
Shutdownbe safe to be called at any point. Even if there is no action to take- If
Shutdownis called beforeRunit will exit cleanly and the next call toRunwill exit due to the previousShutdown - If
Shutdownis called whileRunis active it should block untilRunhas confirmed shutdown.
I went with waitgroup approach because i could think of 3 options how Run and Shutdown can be called.
ShutdownbeforeRun: no need to wait for confirmation pipelines not set upShutdownduringRunwe wait for all run to finish workRunfinished beforeShutdownNo need to wait as nobody will handle shutdown signal until nextRunis called for same collector. Also in this case i'm not sure it's viable case. It looks like Run on exit stops the service.
From these situations it looks that it's enough to wait for Runs to finish. Other way would be pulling state of Config provider and Service until we have a Shutdown match. This could possibly block in case of no Run command running with the same effect as WaitGroup approach but it would take longer. On top of that we would need to have 2 or 3 states for each of these components (not started[optional], running, stopped)
Link to tracking Issue: #4947
Testing: Unit tests
As for incoming release, I'm raising this mainly for discussion and I'll keep it up to date until it's possible to merge
Codecov Report
Attention: 7 lines in your changes are missing coverage. Please review.
Comparison is base (
6ed7ad1) 90.80% compared to head (e8c4c5b) 90.79%. Report is 150 commits behind head on main.
:exclamation: Current head e8c4c5b differs from pull request most recent head eb38c49. Consider uploading reports for the commit eb38c49 to get more accurate results
| Files | Patch % | Lines |
|---|---|---|
| otelcol/collector.go | 73.07% | 5 Missing and 2 partials :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #8811 +/- ##
==========================================
- Coverage 90.80% 90.79% -0.02%
==========================================
Files 341 318 -23
Lines 18330 17415 -915
==========================================
- Hits 16645 15812 -833
+ Misses 1350 1306 -44
+ Partials 335 297 -38
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This PR was marked stale due to lack of activity. It will be closed in 14 days.
hey @bogdandrutu any feedback would be appreciated. does direction took seem ok?
This PR was marked stale due to lack of activity. It will be closed in 14 days.
This PR was marked stale due to lack of activity. It will be closed in 14 days.
This PR was marked stale due to lack of activity. It will be closed in 14 days.
Closed as inactive. Feel free to reopen if this PR is still being worked on.