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

ParentBasedSampler should not explicitly consider Links #1851

Merged
merged 7 commits into from
Mar 6, 2021

Conversation

mbakalov
Copy link
Contributor

Fixes #1846.

Changes

Previously we had functionality in ParentBasedSampler that would consider activity links and return RecordAndSample if one of the links was sampled. This isn't according to the spec and is removed in this PR.

In a related PR we've added customization capability to ParentBasedSampler that can be used to examine links if needed.

  • CHANGELOG.md updated for non-trivial changes

@mbakalov
Copy link
Contributor Author

mbakalov commented Feb 25, 2021

Just waiting for the related PR to get merged before making changes to the actual sampler.

@mbakalov mbakalov marked this pull request as ready for review February 27, 2021 18:48
@mbakalov mbakalov requested a review from a team February 27, 2021 18:48
@codecov
Copy link

codecov bot commented Feb 27, 2021

Codecov Report

Merging #1851 (255ddc7) into main (cb066cb) will decrease coverage by 0.48%.
The diff coverage is 78.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1851      +/-   ##
==========================================
- Coverage   83.77%   83.29%   -0.49%     
==========================================
  Files         187      189       +2     
  Lines        5967     6123     +156     
==========================================
+ Hits         4999     5100     +101     
- Misses        968     1023      +55     
Impacted Files Coverage Δ
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 26.82% <20.54%> (-44.60%) ⬇️
src/OpenTelemetry/Logs/LogRecord.cs 82.50% <77.77%> (-5.00%) ⬇️
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 77.55% <88.23%> (+7.09%) ⬆️
...rc/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs 88.46% <88.88%> (-2.28%) ⬇️
src/OpenTelemetry/Trace/ExceptionProcessor.cs 90.90% <90.90%> (ø)
src/OpenTelemetry/Trace/TracerProviderSdk.cs 95.16% <98.64%> (+4.38%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 86.89% <100.00%> (+0.05%) ⬆️
...ry.Instrumentation.AspNet/AspNetInstrumentation.cs 100.00% <100.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs 89.10% <100.00%> (ø)
...entation.AspNet/TracerProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
... and 24 more

Copy link
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.

@cijothomas
Copy link
Member

@mbakalov thanks! please resolve conflict and we are good to merge.

@mbakalov mbakalov closed this Mar 6, 2021
@mbakalov mbakalov reopened this Mar 6, 2021
@mbakalov
Copy link
Contributor Author

mbakalov commented Mar 6, 2021

@mbakalov thanks! please resolve conflict and we are good to merge.

Thanks @cijothomas - done now!

@cijothomas cijothomas merged commit 21c440a into open-telemetry:main Mar 6, 2021
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.

ParentBasedSampler should not make decision based on Links
3 participants