Skip to content

fix TracerProvider constructor#177

Merged
reyang merged 5 commits into
open-telemetry:masterfrom
ziqizh:tracerprovider-fix
Jul 20, 2020
Merged

fix TracerProvider constructor#177
reyang merged 5 commits into
open-telemetry:masterfrom
ziqizh:tracerprovider-fix

Conversation

@ziqizh
Copy link
Copy Markdown
Contributor

@ziqizh ziqizh commented Jul 16, 2020 •
edited
Loading

Current tracer provider doesn't set the sampler for the tracer, which caused the underlying tracer instance always use AlwaysOn sampler.
I also wrote an comprehensive unit test to test its functionality.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 16, 2020 •
edited
Loading

Codecov Report

Merging #177 into master will increase coverage by 0.57%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #177      +/-   ##
==========================================
+ Coverage   94.88%   95.46%   +0.57%     
==========================================
  Files          84       87       +3     
  Lines        2171     2492     +321     
==========================================
+ Hits         2060     2379     +319     
- Misses        111      113       +2
Impacted Files Coverage Δ
sdk/src/trace/tracer_provider.cc 53.33% <100.00%> (ø)
sdk/test/trace/tracer_provider_test.cc 100.00% <100.00%> (ø)
...nclude/opentelemetry/ext/zpages/tracez_processor.h 100.00% <0.00%> (ø)
ext/src/zpages/tracez_processor.cc 100.00% <0.00%> (ø)
ext/test/zpages/tracez_processor_test.cc 99.30% <0.00%> (ø)

jsandler18
jsandler18 approved these changes Jul 16, 2020
View reviewed changes
Copy link
Copy Markdown

@jsandler18 jsandler18 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ziqizh ziqizh marked this pull request as ready for review July 16, 2020 14:52
@ziqizh ziqizh requested a review from a team July 16, 2020 14:52
reyang
reyang approved these changes Jul 16, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@reyang reyang 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
reyang reviewed Jul 16, 2020
View reviewed changes
Comment thread sdk/test/trace/tracer_provider_test.cc Outdated
@reyang reyang added the pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.) label Jul 16, 2020
oulin-coder
oulin-coder approved these changes Jul 16, 2020
View reviewed changes
Copy link
Copy Markdown

@oulin-coder oulin-coder left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just one question.

Comment thread sdk/test/trace/tracer_provider_test.cc
@reyang reyang merged commit 39ac11b into open-telemetry:master Jul 20, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
@malkia
Merge pull request open-telemetry#177 from malkia/renovate/rules_appl…
e383ea1 
…e-3.x

Update dependency rules_apple to v3.18.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@reyang reyang reyang approved these changes

+3 more reviewers

@jsandler18 jsandler18 jsandler18 approved these changes

@pyohannes pyohannes pyohannes approved these changes

@oulin-coder oulin-coder oulin-coder approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr:please-merge This PR is ready to be merged by a Maintainer (rebased, CI passed, has enough valid approvals, etc.)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ziqizh @jsandler18 @pyohannes @reyang @oulin-coder