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

Collector Run will listen for context done #4954

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

cpheps
Copy link
Contributor

@cpheps cpheps commented Mar 3, 2022

Description:
Fixes #4842

Added listening for <-ctx.Done() in Run select so if the passed in context cancels the collector will too.

There was a weird pattern though where internal shutdown takes a context and currently it's the passed in one. For the case of context cancelling I called the shutdown function with background context. Wondering if we want to put a timer on this or if we want to always use background context on shutdown. I could see a scenario where the collector is shutting down and the passed in context is canceled and interrupts the graceful shutdown.

Link to tracking Issue: #4842

Testing: Added unit test

@cpheps cpheps requested review from a team and dmitryax March 3, 2022 19:50
Signed-off-by: Corbin Phelps <[email protected]>
@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #4954 (eb8f911) into main (c257aba) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4954   +/-   ##
=======================================
  Coverage   91.03%   91.04%           
=======================================
  Files         178      178           
  Lines       10613    10617    +4     
=======================================
+ Hits         9662     9666    +4     
  Misses        734      734           
  Partials      217      217           
Impacted Files Coverage Δ
service/collector.go 77.43% <100.00%> (+0.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c257aba...eb8f911. Read the comment docs.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

I am confused if we need Shutdown at all at this point. I think this is a good change, but would like to think about.

@cpheps
Copy link
Contributor Author

cpheps commented Mar 3, 2022

I am confused if we need Shutdown at all at this point. I think this is a good change, but would like to think about.

I think it's still useful to have Shutdown. Since the collector is likely the core thing being run in a binary containing it I would think most use cases for context would be from signal intercepts. If you didn't want to intercept signals Shutdown provides a clear way to stop the collector. I think using context or Shutdown achieve the same behavior but calling Shutdown is more deliberate on the part of the wrapper code.

@bogdandrutu bogdandrutu merged commit 93272a5 into open-telemetry:main Mar 3, 2022
@cpheps cpheps deleted the run-respect-context branch March 3, 2022 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collector Run does not listen for context Done
2 participants