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

Don't panic when setting a provider to itself #2749

Merged
merged 19 commits into from Mar 31, 2022
Merged

Don't panic when setting a provider to itself #2749

merged 19 commits into from Mar 31, 2022

Conversation

dmathieu
Copy link
Member

Fixes #2682

This avoids panicking when setting a TracerProvider, MeterProvider or TextMapPropagator to itself, but returns early instead, so a noop happens.

While doing this, I also added better tests for the trace state, so they align with metrics tests.

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #2749 (d7cfc98) into main (60041d2) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2749   +/-   ##
=====================================
  Coverage   76.7%   76.8%           
=====================================
  Files        181     181           
  Lines      12178   12189   +11     
=====================================
+ Hits        9352    9367   +15     
+ Misses      2601    2597    -4     
  Partials     225     225           
Impacted Files Coverage Δ
internal/global/state.go 100.0% <100.0%> (ø)
metric/internal/global/state.go 100.0% <100.0%> (ø)
internal/global/internal_logging.go 75.0% <0.0%> (+25.0%) ⬆️

CHANGELOG.md Outdated Show resolved Hide resolved
metric/internal/global/state_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@dmathieu
Copy link
Member Author

The CI test failure seems unrelated (I can't restart it though).

metric/internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
metric/internal/global/state_test.go Outdated Show resolved Hide resolved
metric/internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
internal/global/state.go Outdated Show resolved Hide resolved
internal/global/state.go Outdated Show resolved Hide resolved
metric/internal/global/state.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
internal/global/state_test.go Outdated Show resolved Hide resolved
metric/internal/global/state_test.go Outdated Show resolved Hide resolved
metric/internal/global/state_test.go Outdated Show resolved Hide resolved
dmathieu and others added 10 commits March 31, 2022 19:26
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MrAlias MrAlias merged commit 625d76d into open-telemetry:main Mar 31, 2022
@dmathieu dmathieu deleted the state-no-panic branch April 1, 2022 07:41
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.

Setting the Global MeterProvider to itself should not panic
5 participants