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

Make TracerProvider not allow to register a SpanProcessor after shutdown #3845

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Mar 8, 2023

Currently isShutdown is not set when spanProcessors is empty. It must always be set because it influences how other methods behave e.g. RegisterSpanProcessor().

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #3845 (259301d) into main (282a47e) will increase coverage by 0.0%.
The diff coverage is 25.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3845   +/-   ##
=====================================
  Coverage   81.7%   81.7%           
=====================================
  Files        170     170           
  Lines      12915   12915           
=====================================
+ Hits       10562   10564    +2     
+ Misses      2134    2132    -2     
  Partials     219     219           
Impacted Files Coverage Δ
sdk/trace/provider.go 87.2% <25.0%> (ø)

... and 1 file with indirect coverage changes

@ash2k ash2k changed the title Always set shutdown flag TraceProvider shutdown tweaks Mar 8, 2023
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Please add a changelog entry (IMO in Fixed section).

@ash2k ash2k force-pushed the set-shutdown-flag branch from d7a0c84 to 41c7941 Compare March 9, 2023 22:47
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

  1. Needs tests
  2. Should be separated into two PRs

@ash2k ash2k force-pushed the set-shutdown-flag branch from 41c7941 to 5b5740e Compare March 22, 2023 08:42
@ash2k ash2k changed the title TraceProvider shutdown tweaks Make TracerProvider to not allow to register a SpanProcessor after shutdown Mar 22, 2023
@ash2k ash2k changed the title Make TracerProvider to not allow to register a SpanProcessor after shutdown Make TracerProvider not allow to register a SpanProcessor after shutdown Mar 22, 2023
@pellared
Copy link
Member

Can you also double check if it fixes #3890?

@ash2k
Copy link
Contributor Author

ash2k commented Mar 22, 2023

@pellared I don't think it does. I don't want to couple two fixes in a single PR to be honest. Especially with reviews being so slow, I'd rather open a separate PR.

CHANGELOG.md Outdated Show resolved Hide resolved
@ash2k ash2k force-pushed the set-shutdown-flag branch from 5b5740e to 353a79f Compare March 22, 2023 09:36
@ash2k ash2k force-pushed the set-shutdown-flag branch from 353a79f to 259301d Compare March 22, 2023 09:37
@MrAlias MrAlias merged commit e4cc478 into open-telemetry:main Mar 22, 2023
@ash2k ash2k deleted the set-shutdown-flag branch March 22, 2023 22:08
@XSAM XSAM added this to the Old Untracked PRs milestone Nov 7, 2024
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.

8 participants