heterogeneous trace context extraction by dgoffredo · Pull Request #72 · DataDog/dd-trace-cpp

Thanks for the thoughtful review, Damien.


I haven't read the RFCs, but it seems that we wanted to process one type of propagation at a time, then now we decide to process several at the same time to the point they can be interchangeable. IMO it would be easier and more efficient to process and validate all propagation style in one pass.

The logic specified in the RFC, as I understand it, is that still we are trying styles in order, but there's a new caveat with regard to the tracestate header, under certain conditions.

DictReader has both lookup and visit because I anticipated a "look at all of the headers in one pass as they're parsed" use case, which is how nginx does things under the hood. However, Envoy exposes a map-like interface. The resulting compromise is that dd-trace-cpp uses a map-like interface, and the map is constructed upfront if necessary (e.g. in nginx).

So, there is little benefit to processing all styles in one pass, and doing so would obscure the fact that the relative priority of configured styles is still important.


Another thing, I noticed that the code to extract traceparent uses std::regex. Although very useful, this library is notoriously known to be very slow. Since traceparent format is easy, we can achieve the same result without it. Here is a quick benchmark.

On the contrary -- std::regex is not slow, it's fast. So that solves that problem!

I considered writing the traceparent parser without regex. tracestate, x-datadog-tags, and most other parsing done by the library is by-hand, without regex. I chose to use regex for traceparent for the following reasons:

  • The nine lines of commented regex pattern at the top of extract_traceparent describe the grammar of what is being parsed, and the parser has no choice but to comply with the grammar.
  • The numeric parsing done after the regex match does not need to be checked for errors, because the correctness of the hex encoding is enforced by the regular expression.
  • Every time I see a hand-written parser, I curse under my breath and reverse engineer the regular expression that it is implementing. There are usually bugs.

Most other parsing done by the library is of the kind "X-separated list of zero or more Y." This does not lend itself particularly well to regex, and is straightforward enough to parse with some careful looping. traceparent, on the other hand, has a known length prefix with a known structure, and so lends itself well to parsing via regex.

I appreciate your working benchmark and alternative non-regex implementation. The alternative implementation (and the adapted regex-based one) does not parse the hex integers, and so the notational burden of that error handling is omitted from your example. Also, your code (which is impressive) requires that I learn the simple machine model of the parser, explain to myself the numeric offsets, and simulate the parse for a variety of examples until I am confident that it does what I like.

Compare this to the regex version, which is pretty much just a single regex pattern.

If you can prove that Envoy or nginx are burdened by this use of std::regex, then I will happily accept your alternative parser. For now, though, let's use std::regex.


I have some immediate changes based on your review that I will push shortly.

Then I'll mull over your review a little longer to see what else I might change.

Then I'll check these changes against Datadog's internal system tests.

Then I'll merge.