128-bit trace IDs#17
Conversation
…dd-trace-cpp into david.goffredo/128-bit-trace-id
cgilmour
left a comment
cgilmour
left a comment
There was a problem hiding this comment.
Approving, with some comments for consideration.
| namespace tracing { | ||
|
|
||
| struct TraceID { | ||
| std::uint64_t low; |
There was a problem hiding this comment.
Setting the initial values to zero here would be another option, instead of doing it in constructors.
| } | ||
|
|
||
| bool operator==(TraceID left, std::uint64_t right) { | ||
| return left == TraceID{right}; |
There was a problem hiding this comment.
It'd be clearer to me if this were defined as left.low == right if that's effectively the outcome.
There was a problem hiding this comment.
left.high == 0 && left.low == right, as I had it before. Then I saw this as better. I see your point, though.
Think I'll keep it as is.
| decision.configured_rate = rule.sample_rate; | ||
| const std::uint64_t threshold = max_id_from_rate(rule.sample_rate); | ||
| if (knuth_hash(span.trace_id) < threshold) { | ||
| if (knuth_hash(span.trace_id.low) < threshold) { |
There was a problem hiding this comment.
So sampling decisions are only affected by the low 64 bits, not the entire value?
Not objecting at all, but it might deserve mentioning in a comment somewhere.
There was a problem hiding this comment.
Yes... maybe in sampling_util, where knuth_hash is defined. Will do.
|
|
||
| if (auto enabled_env = | ||
| lookup(environment::DD_TRACE_128_BIT_TRACEID_ENABLED)) { | ||
| result.trace_id_128_bit = !falsy(*enabled_env); |
There was a problem hiding this comment.
Just for amusement, what's the outcome for DD_TRACE_128_BIT_TRACEID_ENABLED being set to "" or anything not specifically "falsy"?
This is answered in the tests, but is that the desired behavior?
There was a problem hiding this comment.
Yes, that's the desired behavior.
As we discussed in our standup yesterday, the falsiness behavior of this library could use revisiting. For now, though, I think that
export DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED=''
should enable the feature, if only because of the word "enabled" in there. Reminds me of the tension in C preprocess macros between #ifdef and #if.
| // trace IDs, extract them from incoming trace context, inject them into | ||
| // outgoing trace context, and send them to the collector. If false, then the | ||
| // tracer will generate 64-bit trace IDs, and extract only the lower 64 bits | ||
| // of trace IDs from incoming trace context. |
There was a problem hiding this comment.
And what does it inject when false? (especially for w3c)
| // of trace IDs from incoming trace context. | ||
| // `trace_id_128_bit` is overridden by the `DD_TRACE_128_BIT_TRACEID_ENABLED` | ||
| // environment variable. | ||
| bool trace_id_128_bit = false; |
There was a problem hiding this comment.
What would be the impact of this being set to true right now, in terms of datadog-only propagation to other tracers?
There was a problem hiding this comment.
It would mean we always send the _dd.p.tid trace tag to the Agent and to forward services. The data cost is marginal, and I wasn't able to measure the latency cost. Still, the spec.
| // ID, padded with zeroes on the left. | ||
| std::string hex_padded() const; | ||
|
|
||
| // Return either a decimal or a "0x"-prefixed hexadecimal representation of |
There was a problem hiding this comment.
Not sure I like this, a straight token is easier to lug around (copy-paste) if it's being used for error diagnostics.
2 participants
This revision introduces 128-bit trace ID generation and propagation for all propagation styles.
It's based on an internal design document.
128-bit generation is disabled by default, and can be enabled via either
bool TracerConfig::trace_id_128_bitorDD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED.