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.

Sign up for GitHub

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

Jump to bottom

TracerProvider cleanups #3942

Merged
merged 4 commits into from
Apr 1, 2023
Merged

TracerProvider cleanups #3942

merged 4 commits into from
Apr 1, 2023

Conversation

ash2k
Copy link
Contributor

@ash2k ash2k commented Mar 28, 2023

Please see individual commits.

dmathieu
dmathieu reviewed Mar 28, 2023
View reviewed changes
sdk/trace/provider.go Outdated Show resolved Hide resolved
@dmathieu dmathieu added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Mar 28, 2023
@codecov
Copy link

codecov bot commented Mar 28, 2023 •
edited
Loading

Codecov Report

Merging #3942 (a574683) into main (271df1d) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3942   +/-   ##
=====================================
  Coverage   81.7%   81.7%           
=====================================
  Files        171     171           
  Lines      12950   12951    +1     
=====================================
+ Hits       10585   10590    +5     
+ Misses      2142    2138    -4     
  Partials     223     223
Impacted Files Coverage Δ
sdk/trace/provider.go 84.3% <100.0%> (+<0.1%) ⬆️
sdk/trace/span.go 87.9% <100.0%> (ø)
sdk/trace/span_processor.go 100.0% <100.0%> (ø)
sdk/trace/tracer.go 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

pellared
pellared approved these changes Mar 28, 2023
View reviewed changes
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.

👍

dmathieu
dmathieu reviewed Mar 28, 2023
View reviewed changes
sdk/trace/provider.go Outdated Show resolved Hide resolved
pellared
pellared requested changes Mar 29, 2023
View reviewed changes
sdk/trace/provider_test.go Outdated Show resolved Hide resolved
pellared
pellared approved these changes Mar 29, 2023
View reviewed changes
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.

PS. Next time I suggest regular commits and merges. Force pushes + rebase on PRs makes reviewing harder as you cannot review only the changes since last review (and you lose the history).

MrAlias
MrAlias reviewed Mar 31, 2023
View reviewed changes
sdk/trace/provider.go Outdated Show resolved Hide resolved
ash2k added 4 commits April 1, 2023 12:08
@ash2k
Pre-allocate spanProcessorStates slice
a644aad
@ash2k
Make sync.Once a non-pointer
15f5701 
It doesn't need to be a pointer, can be part of the struct to avoid allocating a separate object for it
@ash2k
getSpanProcessors() helper
6395dff
@ash2k
Add tests for UnregisterSpanProcessor()
a574683
@MrAlias MrAlias merged commit 22fd104 into open-telemetry:main Apr 1, 2023
@ash2k ash2k deleted the ash2k/cleanups branch April 1, 2023 23:11
@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
Reviewers

@hanyuancheung hanyuancheung hanyuancheung approved these changes

@MrAlias MrAlias MrAlias approved these changes

@pellared pellared pellared approved these changes

@dmathieu dmathieu dmathieu approved these changes

@jmacd jmacd Awaiting requested review from jmacd

@Aneurysm9 Aneurysm9 Awaiting requested review from Aneurysm9

@evantorrie evantorrie Awaiting requested review from evantorrie

@XSAM XSAM Awaiting requested review from XSAM XSAM is a code owner

@dashpole dashpole Awaiting requested review from dashpole dashpole is a code owner

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

Assignees
No one assigned
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
untracked
Development

Successfully merging this pull request may close these issues.
6 participants
@ash2k @dmathieu @pellared @MrAlias @hanyuancheung @XSAM