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

Propagate sample rate through tracestate #1147

Merged
merged 7 commits into from
Mar 26, 2021

Conversation

russcam
Copy link
Contributor

@russcam russcam commented Jan 29, 2021

This commit propagates the sample rate of the APM agent through the tracestate header, and uses the sample rate received in incoming tracestate as the sample rate for non-root transactions and spans.

Introduce TraceState type to model tracestate, allowing incoming headers to be added and sample rate to be set. Move TraceState validation from TraceContext onto TraceState.

Obsolete TryDeserializeFromString and SerializeToString on DistributedTracingData in favour of a method that accepts both traceparent and tracestate, and properties that get the traceparent and tracestate text headers, respectively.

Change Transaction and Span SampleRate to double? to allow a value to be omitted when incoming Trace context does not contain a tracestate header or the header does not contain the es vendor s attribute.

Closes #1021

@russcam russcam added enhancement New feature or request agent-dotnet v1.8.0 labels Jan 29, 2021
@apmmachine
Copy link
Contributor

apmmachine commented Jan 29, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #1147 updated

  • Start Time: 2021-03-25T23:22:06.676+0000

  • Duration: 56 min 4 sec

  • Commit: 97aa519

Test stats 🧪

Test Results
Failed 0
Passed 19089
Skipped 80
Total 19169

Trends 🧪

Image of Build Times

Image of Tests

@russcam russcam requested a review from gregkalapos February 9, 2021 05:19
This commit propagates the sample rate of the
APM agent through the tracestate header, and
uses the sample rate received in incoming tracestate
as the sample rate for non-root transactions and spans.

Introduce TraceState type to model tracestate,
allowing incoming headers to be added and sample
rate to be set. Move TraceState validation from
TraceContext onto TraceState.

Obsolete TryDeserializeFromString and SerializeToString
on DistributedTracingData in favour of a method that accepts
both traceparent and tracestate, and properties that get
the traceparent and tracestate text headers, respectively.

Change Transaction and Span SampleRate to double?
to allow a value to be omitted when incoming
Trace context does not contain a tracestate header
or the header does not contain the es vendor s attribute.

Closes elastic#1021
@russcam russcam force-pushed the feature/propagate-samplerate branch from 8ea2adf to c8d3110 Compare February 11, 2021 02:13
@russcam russcam added v1.9.0 and removed v1.8.0 labels Feb 16, 2021
Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

I have a bit of a mixed feeling about adding [Obsolete()] to the DistributedTracingData public APIs.

For HTTP we propagate the tracestate out of the box and I'm not sure how relevant it is to propagate this value manually for other use cases. I usually tend to prefer not to change public API. I'd totally agree with this PR if we'd start out new, but I'm not sure the benefit of adding the ability to propagate tracestate manually outweighs technically breaking user code (sure, with the attribute we don't break directly, but long term users need to change user code).

I'll ask around on what other agents do.

@russcam
Copy link
Contributor Author

russcam commented Mar 9, 2021

In its current form, DistributedTracingData only exposes traceparent and not tracestate. The Elastic APM agents may be used with other systems that require manual propagation of traceparent and tracestate, but they would only be able to get the former and not the latter, which on the surface seems limiting; for example, System.Diagnostics.ActivityContext exposes TraceState.

Let's sync to discuss.

russcam and others added 2 commits March 23, 2021 15:46
This commit removes the Obsolete attributes from DistributedTracingData,
and does not expose Tracestate for now. Can evaluate again in future.

When an activity is created when a transaction is started, use the tracestate
from the activity, if present, but set the sample rate from the sampler and
mutate tracestate if the es vendor is present. A transaction created without
explicitly passed DistributedTracingData is considered to be a root transaction,
so the sample rate is governed by the sampler.
@russcam
Copy link
Contributor Author

russcam commented Mar 24, 2021

Would you mind taking another look at this, @gregkalapos? I think it's good to go now, with the changes in 9c6d5f3, following our discussion.

@gregkalapos gregkalapos self-requested a review March 25, 2021 20:08
Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Just to document here one thing I was interested in and played with:
Activity also propagates the traceparent and tracestate in HTTP requests - so it writes the exact same HTTP headers as we do. I did some testing and it seems that always our values end up being in those headers. We specifically check in HttpDiagnosticListenerImplBase.cs if those hears are already present and only write those if they aren't. As it seems this is all fine and works as intended.

@russcam russcam merged commit c978e42 into elastic:master Mar 26, 2021
@russcam russcam deleted the feature/propagate-samplerate branch March 26, 2021 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Propagate sample rate in tracestate
3 participants