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

Expose utils to parse trace state #934

Closed
lmolkova opened this issue Jul 27, 2020 · 4 comments
Closed

Expose utils to parse trace state #934

lmolkova opened this issue Jul 27, 2020 · 4 comments
Labels
enhancement New feature or request

Comments

@lmolkova
Copy link

lmolkova commented Jul 27, 2020

Is your feature request related to a problem?
Some custom samplers need to access tracestate to make sampling decision (this is on of the ways to interop with legacy custom correlation protocols). Currently such samplers would have to parse tracestate on their own.
Let's say sampler whats to check certain flag value in tracestate: 'sampling.priority=0.5'

  • Sampler implements SamplingResult ShouldSample(in SamplingParameters samplingParameters)
  • SamplingResult has ActivityContext which has TraceState string
  • Tracestate parsing methods are not exposed and samplers should parse it on their own

Describe the solution you'd like.

  • Expose TracestateUtils and add static public bool TryExtractTracestate (string tracestateString, out IEnumerable<KeyValuePair<string, string>> tracestate)

Describe alternatives you've considered.

  • let it be (custom implementations will likely be error prone and less efficient). At the same time it's likely samplers can do cheaper parsing of the single flag they need.
  • Add Tracestate type and have static parsing methods on it. I believe we should keep public API area smaller and we pushed hard in the past to remove any types that are not essential, so this seems to be an overkill.
  • Expose SpanContext(ActivityContext) or add SpanContext(traceid, spanId, flags, isRemote, tracestateString)ctor: not great since samplers don't need SpanContext, just the tracestate portion of it
@cijothomas
Copy link
Member

namespace System.Diagnostics
{
    public readonly partial struct ActivityContext : IEquatable<ActivityContext>
    {
+        public static bool TryParse(string w3cId, string? traceState, out ActivityContext context);
    }
}

The above is coming in .NET API. It'll do entire ActivityContext, not just tracestate portion.

@lmolkova
Copy link
Author

@cijothomas yes!!!

@lmolkova
Copy link
Author

lmolkova commented Aug 3, 2020

been thinking about it and decided to reopen:
Yes. I can use ActivityContext.Parse and then create SpanContext out of it and iterate over tracestate, but actually the only thing I need is parsed tracestate.

So this seems to be a reasonable workaround, but parsing traceparent is an overhead that ideally should be avoided for the sake of perf.

@lmolkova lmolkova reopened this Aug 3, 2020
@martinjt
Copy link
Member

I don't think we have intentions to add this. You can parse the whole traceparent and tracestate using the TextMapPropagator, or through the Activity methods.

If you feel strongly that it should be provided by OpenTelemetry, then please reopen. I don't think this is a requirement for the SDK via the specifications though.

@martinjt martinjt closed this as not planned Won't fix, can't repro, duplicate, stale Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants