Support for sampling priority

[quentin-]

👋,

I'm new to DD, but it seems like most other DD libraries have some support for this.
Is adding this to the node library in the roadmap?

Thank you.

[rochdev]

This is definitely on the roadmap. I'll keep this issue updated when we start working on it.

[stephenh]

@rochdev does dd-trace-js currently support any distributed tracing? E.g. reading/writing x-datadog-trace-id/span-id headers? I did a quick grep and it doesn't look like it, but may have missed it.

[rochdev]

@stephenh Yes, it is supported by using OpenTracing's inject and extract methods which will handle these headers for you. Supported modules like http and express will also handle this automatically.

For our implementation, see src/opentracing/propagation/text_map.js

[stephenh]

@rochdev ah thanks! Apologies, I did something really dumb when searching for the headers.

Any guesses about how far out sampling priority is? We just start poking around at "have engineers opt-in their requests for end-to-end distributed tracing" (we have too many requests to log everything), and basically everything goes through graphql, so it won't be terribly useful until dd-trace-js reads/forwards the x-datadog-sampling-priority header.

[stephenh]

@rochdev I'd really like to have this. Can I submit a PR? In theory just watch for the incoming header and then have sampler somehow check the span's context? Not entirely sure how to get from span -> the text map-created context.

[rochdev]

@stephenh Of course! If you prefer to wait we plan to implement this fairly soon as well.

For the headers you should be able to look at any one of our tracers that supports sampling priority and implement the same logic.

About your last point, I'm not sure I understand correctly what you mean. Can you clarify?

[stephenh]

@rochdev sweet. How soon is soon? :-) If it's "not this week", I might poke around.

About your last point, I'm not sure I understand correctly what you mean

Well, I was looking at Sampler's isSampled(span) method and just thinking I'd want to do something like span.context.theSamplingPriorityHeader == 2 (or what not).

FWIW I haven't looked at other impls yet, but I mainly just want this to collect + respect + pass-along the x-datadog-sampling-priority header, as apollo is already in the middle of our trace flow, and not necessarily initiative the x-datadog-sampling-priority value, if that makes it any simpler (and is probably what I'd do as my 1st pass PR).

[rochdev]

@stephenh In the coming weeks but definitely not this week.

Makes sense for the header. There is not much more than that to it either I think, but feel free to open a PR and we'll discuss if there are any adjustments to make.

[stephenh]

I'm poking around a bit. I'm curious about this line:

https://github.com/DataDog/dd-trace-js/blob/master/src/opentracing/span_context.js#L12

AFAICT in span.createContext, sampled is always passed (e.g. creating a new span from another span, or a new top-level trace/span).

But in the text_map propagator, sampled is not passed, which to me insinuates sampled === undefined will always be true and so any incoming trace will be automatically sampled?

That doesn't sound right?