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

Add support for samplers to modify Tracestate #3610

Merged

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Aug 27, 2022

Fixes #1708
To maintain backward compatibility, SamplingResult will default to null for TraceStateString. Any non-null value set by samplers, will be set as the tracestate of the created activity.
If sampler intents to not modify tracestate, it can return null. (Default)
Else, it can populate tracestate with desired value.
Given tracestate is implemented as a opaque string, the samper is responsible for ensuring the tracestate semantics are followed.

@cijothomas cijothomas requested a review from a team as a code owner August 27, 2022 06:15
@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #3610 (745a9c6) into main (e6d39b8) will decrease coverage by 0.07%.
The diff coverage is 86.95%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3610      +/-   ##
==========================================
- Coverage   87.22%   87.14%   -0.08%     
==========================================
  Files         280      280              
  Lines       10056    10073      +17     
==========================================
+ Hits         8771     8778       +7     
- Misses       1285     1295      +10     
Impacted Files Coverage Δ
src/OpenTelemetry/Trace/SamplingResult.cs 88.57% <80.00%> (-11.43%) ⬇️
src/OpenTelemetry/Trace/TracerProviderSdk.cs 99.23% <100.00%> (+0.40%) ⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 50.00% <0.00%> (-28.58%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 59.09% <0.00%> (-18.19%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 91.75% <0.00%> (-3.30%) ⬇️
...tpListener/Internal/PrometheusCollectionManager.cs 78.04% <0.00%> (-2.44%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 77.50% <0.00%> (+0.83%) ⬆️
....Prometheus.HttpListener/PrometheusHttpListener.cs 82.66% <0.00%> (+4.00%) ⬆️
... and 3 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.

Added couple minor suggestions.
LGTM with a changelog entry.

@cijothomas cijothomas merged commit b54cc80 into open-telemetry:main Aug 29, 2022
@cijothomas cijothomas deleted the cijothomas/tracestatesampelr branch August 29, 2022 23:36
@@ -42,6 +43,7 @@ public SamplingResult(bool isSampled)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need an overload for this one?

Copy link
Member Author

Choose a reason for hiding this comment

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

its optional. if there is a need, we can always add more overloads in future.

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.

Samplers should be allowed to modify tracestate
4 participants