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

Call sampler on local child spans. #1233

Merged
merged 4 commits into from
Oct 9, 2020
Merged

Call sampler on local child spans. #1233

merged 4 commits into from
Oct 9, 2020

Conversation

paulosman
Copy link
Member

Fixes #1228

Previously, the sampler would only be called for the first span in a process. This makes sense for trace level sampling but doesn't allow the sampler to specify attributes to be set on each span, which is important for some implementations. This change removes the guard that limits sampling decisions to local root spans.

@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #1233 into master will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1233   +/-   ##
======================================
  Coverage    76.7%   76.7%           
======================================
  Files         130     130           
  Lines        5832    5828    -4     
======================================
  Hits         4475    4475           
+ Misses       1110    1105    -5     
- Partials      247     248    +1     
Impacted Files Coverage Δ
sdk/trace/span.go 92.9% <100.0%> (-0.2%) ⬇️
sdk/trace/batch_span_processor.go 78.4% <0.0%> (+4.3%) ⬆️

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@MrAlias MrAlias added area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package priority:p2 labels Oct 8, 2020
@MrAlias MrAlias added this to the RC1 milestone Oct 8, 2020
@MrAlias
Copy link
Contributor

MrAlias commented Oct 9, 2020

@paulosman it looks like maintainer edits are turned off for this PR so I can't merge master into this. If you can do this sync this PR looks ready to merge.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@lizthegrey lizthegrey left a comment

Choose a reason for hiding this comment

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

please write tests for this.

CHANGELOG.md Outdated Show resolved Hide resolved
@paulosman
Copy link
Member Author

please write tests for this.

Added a test to catch local child spans. Build is fixed, but circle seems stuck - pushed a changed and it won't rebuild.

@Aneurysm9 Aneurysm9 merged commit 02cd123 into open-telemetry:master Oct 9, 2020
@MrAlias MrAlias mentioned this pull request Nov 20, 2020
AzfaarQureshi pushed a commit to open-o11y/opentelemetry-go that referenced this pull request Dec 3, 2020
* Call sampler on local child spans.

* Update CHANGELOG.md

* Adding a test for calling the sampler on local child spans

* Add clarifying comment to test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attributes from Samplers not being added to sampled Spans
5 participants