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 span IsRecording when not sampling #1750

Merged
merged 12 commits into from
Mar 30, 2021

Conversation

bryan-aguilar
Copy link
Contributor

The span.IsRecording() call would incorrectly return true even if the Sampler.ShouldSample() == DROP.

An issue also arose in TestSampling test case after the original issue was resolved. The test case checked that all three executionTracerTaskEnd functions would be executed instead of just one. These function calls are never executed if the span was not recorded.

Fixes #1664

bryan-aguilar and others added 8 commits March 26, 2021 15:30
…correct span.isRecording logic to return false when not being sampled
tests and check for when span is ended immediately
Improve readability of test name

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Improve readability of test name

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 29, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #1750 (156686a) into main (20c93b0) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1750     +/-   ##
=======================================
+ Coverage   78.0%   78.1%   +0.1%     
=======================================
  Files        132     132             
  Lines       6948    6948             
=======================================
+ Hits        5422    5432     +10     
+ Misses      1276    1270      -6     
+ Partials     250     246      -4     
Impacted Files Coverage Δ
sdk/trace/span.go 94.0% <100.0%> (+3.6%) ⬆️
exporters/otlp/otlpgrpc/connection.go 88.7% <0.0%> (+1.8%) ⬆️

CHANGELOG.md Outdated Show resolved Hide resolved
sdk/trace/trace_test.go Outdated Show resolved Hide resolved
bryan-aguilar and others added 2 commits March 29, 2021 13:20
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MrAlias
Copy link
Contributor

MrAlias commented Mar 30, 2021

@bryan-aguilar updates from maintainer are off for this PR so I'm not able to merge in main. Can you do this so we can merge this PR?

@bryan-aguilar
Copy link
Contributor Author

@MrAlias Updated, sorry about that.

@MrAlias MrAlias merged commit d575865 into open-telemetry:main Mar 30, 2021
@MrAlias MrAlias mentioned this pull request Apr 23, 2021
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

span.IsRecording() returns true even when Sampler indicates it should be DROPPED
5 participants