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

fix: respect sampled flag in Span Processors, fix associated tests #2396

Merged

Conversation

quickgiant
Copy link
Contributor

Which problem is this PR solving?

Fixes #2394

Short description of the changes

Span Processor classes were missing a check on trace flags to determine whether spans are sampled and should be exported, as per the spec here: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#sampling. These classes originally performed this check and in the case of SimpleSpanProcessor, there is actually a unit test for the behavior that was incorrectly passing due to an issue regarding a shared exporter instance across tests. Since the shared exporter was shut down in the first test, the unsampled span was being dropped, causing the test to pass for the wrong reason.

This change restores the trace flags checks, fixes the SimpleSpanProcessor test, and adds a new test for BatchSpanProcessor. No change is needed for MultiSpanProcessor because the individual span processors ought to do the checks themselves.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 3, 2021

CLA Signed

The committers are authorized under a signed CLA.

@dyladan dyladan assigned dyladan and unassigned dyladan Aug 4, 2021
@dyladan dyladan added the bug Something isn't working label Aug 4, 2021
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #2396 (0b1748f) into main (f180870) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2396      +/-   ##
==========================================
- Coverage   92.80%   92.79%   -0.02%     
==========================================
  Files         145      145              
  Lines        5226     5230       +4     
  Branches     1071     1073       +2     
==========================================
+ Hits         4850     4853       +3     
- Misses        376      377       +1     
Impacted Files Coverage Δ
...dk-trace-base/src/export/BatchSpanProcessorBase.ts 91.57% <100.00%> (+0.18%) ⬆️
...y-sdk-trace-base/src/export/SimpleSpanProcessor.ts 83.87% <100.00%> (+1.11%) ⬆️
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@quickgiant quickgiant force-pushed the issue-2394-respect-sampled branch from 5481af4 to f9db7e5 Compare August 4, 2021 18:31
@quickgiant quickgiant force-pushed the issue-2394-respect-sampled branch from f9db7e5 to 9d66288 Compare August 4, 2021 19:12
@Flarna
Copy link
Member

Flarna commented Aug 4, 2021

@quickgiant Please avoid forced pushes as this makes reviewing of changes harder. It's fine to add a lot commits and merge main as needed. During integration to main all commits on your branch will be squashed into a single one.

@quickgiant quickgiant requested a review from a team August 5, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Span Processors do not respect sampled flag when exporting
6 participants