Skip to content

Move SpanContext to Separate File#168

Merged
reyang merged 12 commits into
open-telemetry:masterfrom
ziqizh:nholbrook/move_span_context
Jul 13, 2020
Merged

Move SpanContext to Separate File#168
reyang merged 12 commits into
open-telemetry:masterfrom
ziqizh:nholbrook/move_span_context

Conversation

@nholbrook
Copy link
Copy Markdown
Contributor

Followup on comment #136 (comment).

Since the ParentOrElse Sampler got merged, the CI tests are throwing errors on my Probability Sampler PR due to duplicate definitions of SpanContext in multiple files. This needs to be merged before #136 can be merged.

Copy link
Copy Markdown
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

Thanks for getting this on the way.

Please place the span_context.h in the api folder, and try to stay close to the interface specified here.

Comment thread sdk/include/opentelemetry/sdk/trace/span_context.h Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/samplers/probability.h Outdated
Comment thread sdk/test/trace/probability_sampler_test.cc Outdated
Comment thread sdk/src/trace/samplers/probability.cc Outdated
Comment thread sdk/include/opentelemetry/sdk/trace/samplers/parent_or_else.h Outdated
@nholbrook nholbrook force-pushed the nholbrook/move_span_context branch from c33c203 to 316e720 Compare July 10, 2020 19:04
@nholbrook nholbrook force-pushed the nholbrook/move_span_context branch from 316e720 to 2130ef0 Compare July 10, 2020 19:10
Comment thread api/include/opentelemetry/trace/span_context.h
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 10, 2020

Codecov Report

Merging #168 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
+ Coverage   94.11%   94.17%   +0.05%     
==========================================
  Files          74       76       +2     
  Lines        1802     1820      +18     
==========================================
+ Hits         1696     1714      +18     
  Misses        106      106              
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/trace/sampler.h 100.00% <ø> (ø)
...lude/opentelemetry/sdk/trace/samplers/always_off.h 100.00% <ø> (ø)
.../opentelemetry/sdk/trace/samplers/parent_or_else.h 100.00% <ø> (ø)
api/include/opentelemetry/trace/span_context.h 100.00% <100.00%> (ø)
api/test/trace/span_context_test.cc 100.00% <100.00%> (ø)
...clude/opentelemetry/sdk/trace/samplers/always_on.h 100.00% <100.00%> (ø)
sdk/src/trace/samplers/parent_or_else.cc 100.00% <100.00%> (ø)
sdk/test/trace/parent_or_else_sampler_test.cc 100.00% <100.00%> (ø)

@nholbrook nholbrook marked this pull request as ready for review July 10, 2020 22:18
@nholbrook nholbrook requested a review from a team July 10, 2020 22:18
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

This looks good. Can we add a TODO in span_context.h to indicate that this is just a stub for now that needs to be revisited?

@pyohannes
Copy link
Copy Markdown
Contributor

That looks good, @reyang please have a look.

This is not yet a fully featured SpanContext implementation, but a stub to unblock sampler work.

Comment thread api/test/trace/span_context_test.cc Outdated
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 merged commit 9902d5e into open-telemetry:master Jul 13, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Jun 17, 2025
…-cpp-1.x

Update dependency prometheus-cpp to v1.3.0.bcr.1
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.

4 participants