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

Update Processor.Shutdown to meet with the latest spec #1282

Merged
merged 2 commits into from Sep 17, 2020

Conversation

reyang
Copy link
Member

@reyang reyang commented Sep 17, 2020

The latest spec requires the processor Shutdown to provide a way to indicate whether it succeeded or not.

Changes

  • Changed ActivityProcessor.Shutdown to return bool.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@reyang reyang requested a review from a team as a code owner September 17, 2020 04:56
}

try
{
this.OnShutdown(timeoutMilliseconds);
return true; // TODO: update exporter.OnShutdown to return boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this return true to the end of this function (right before })?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I look at try { return } catch() { return } is similar to if() { return } else { return }. Do you see some benefit of putting it outside?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought normal execution flow would end at }, besides that, I don't see any advantage to move return true to the end of the function.

@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

Merging #1282 into master will decrease coverage by 0.02%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1282      +/-   ##
==========================================
- Coverage   79.13%   79.10%   -0.03%     
==========================================
  Files         215      215              
  Lines        6174     6170       -4     
==========================================
- Hits         4886     4881       -5     
- Misses       1288     1289       +1     
Impacted Files Coverage Δ
src/OpenTelemetry/Trace/ActivityExporter.cs 66.66% <33.33%> (ø)
src/OpenTelemetry/Trace/ActivityProcessor.cs 58.33% <50.00%> (-1.67%) ⬇️
...OpenTelemetry/Trace/BaseExportActivityProcessor.cs 78.57% <100.00%> (-1.43%) ⬇️
...penTelemetry/Trace/BatchExportActivityProcessor.cs 93.05% <100.00%> (-0.28%) ⬇️
.../OpenTelemetry/Trace/CompositeActivityProcessor.cs 92.95% <100.00%> (+0.10%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 61.76% <0.00%> (-2.95%) ⬇️

@CodeBlanch
Copy link
Member

Interesting! What is user to do if shutdown comes back false?

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@reyang
Copy link
Member Author

reyang commented Sep 17, 2020

Interesting! What is user to do if shutdown comes back false?

I guess 99% users won't consume the return value.
For folks who build nuclear power plants, they could leverage the boolean value and decide if it is time for emergency escalation 😄

One real world scenario: one can check the return value == true and write a local file indicating the clean shutdown, when the app started/restarted, it could check if there was a clean shutdown or not.

@cijothomas cijothomas merged commit 8a4621c into master Sep 17, 2020
@cijothomas cijothomas deleted the reyang/shutdown branch September 17, 2020 16:26
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.

None yet

4 participants