opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

Synchronized Run and Shutdown for otelcol/Collector

Open michalpristas opened this issue 2 years ago • 6 comments
trafficstars

Description: Adding some synchronization for Run and Shutdown commands in otelcol/collector.go Should deal with all 3 requirements from the issue:

  1. Shutdown be safe to be called at any point. Even if there is no action to take
  2. If Shutdown is called before Run it will exit cleanly and the next call to Run will exit due to the previous Shutdown
  3. If Shutdown is called while Run is active it should block until Run has confirmed shutdown.

I went with waitgroup approach because i could think of 3 options how Run and Shutdown can be called.

  • Shutdown before Run: no need to wait for confirmation pipelines not set up
  • Shutdown during Run we wait for all run to finish work
  • Run finished before Shutdown No need to wait as nobody will handle shutdown signal until next Run is 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

michalpristas avatar Nov 07 '23 13:11 michalpristas

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.

codecov[bot] avatar Nov 11 '23 02:11 codecov[bot]

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

github-actions[bot] avatar Nov 25 '23 03:11 github-actions[bot]

hey @bogdandrutu any feedback would be appreciated. does direction took seem ok?

michalpristas avatar Dec 01 '23 11:12 michalpristas

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

github-actions[bot] avatar Dec 28 '23 03:12 github-actions[bot]

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

github-actions[bot] avatar Jan 25 '24 03:01 github-actions[bot]

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

github-actions[bot] avatar Feb 10 '24 03:02 github-actions[bot]

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

github-actions[bot] avatar Feb 29 '24 03:02 github-actions[bot]