Conversation
8a8a7b6 to
343d9ea
Compare
98a9a8e to
c1d099f
Compare
5f82e62 to
f789c1c
Compare
cgilmour
left a comment
There was a problem hiding this comment.
I'm marking this as approved for now, with some questions in the comments.
Some extra time would be needed to dig into the implementation details / cross-check with opentelemtry specs and other implementations.
Especially with regard to extraction, I think as other implementations are being completed, it'd be good to have some interop tests with other tracers developed by datadog and also external implementations.
In that circumstance, what's the overall impact on tracing if we received this header but didn't perfectly comprehend it?
| // without any leading zeroes. | ||
| template <typename Integer> | ||
| std::string hex(Integer value) { | ||
| // 4 bits per hex digit char, and then +1 char for possible minus sign |
There was a problem hiding this comment.
Negative hex values shouldn't be a thing here, since it's intended to be an encoding of a bit pattern (more like a hexdump) instead of a representation of a numeric value.
There was a problem hiding this comment.
This function supports any integral type, so I had to accommodate the minus sign.
In a more recent PR, I constrained the type to be unsigned and added a "padded" variant of the function: https://github.com/DataDog/dd-trace-cpp/pull/17/files#diff-666374b8256a8b91fa9a3a030a39c3f75cfafe26d044c6c9c7d9e39db6c895f8R32-R51
Would you like me to backport that change on this PR?
| message += std::to_string(encoded_trace_tags.size()); | ||
| message += " bytes."; | ||
| logger.log_error(message); | ||
| local_root_tags[tags::internal::propagation_error] = "inject_max_size"; |
There was a problem hiding this comment.
Why on local-root instead of the span specifically?
What if the same span is propagated more than once?
What if multiple child spans of the local-root experienced the same error?
There was a problem hiding this comment.
tags::internal::propagation_error is specified in the horizontal propagation RFC. There it's defined to be set on the local root span, and implementations are allowed to either keep the first error or to keep the last error. I chose the latter.
This section of code refactors behavior that existed before this PR.
| const Optional<std::string>& hostname, Optional<std::string> origin, | ||
| std::size_t tags_header_max_size, | ||
| std::unordered_map<std::string, std::string> trace_tags, | ||
| std::vector<std::pair<std::string, std::string>> trace_tags, |
There was a problem hiding this comment.
That's a lot of parameters for a constructor. Just sayin.
There was a problem hiding this comment.
Yes, I could create a struct to hold them, but I decided not to. What do you think?
| // - equal to one of the `disallowed_characters`. | ||
| // | ||
| // `verboten` is used with `std::replace_if` to sanitize field values within the | ||
| // tracestate header. |
There was a problem hiding this comment.
The entire header or just our part of it?
There was a problem hiding this comment.
Just our part of it.
| result += *additional_datadog_w3c_tracestate; | ||
| } | ||
|
|
||
| const std::size_t max_size = 256; |
There was a problem hiding this comment.
Is this an arbitrary size or is it defined somewhere?
There was a problem hiding this comment.
It's defined in our W3C propagation RFC. It's not configurable because it is derived from the W3C definition itself.
|
Thanks for the review.
There is or will be system tests for this feature. As a prerequisite to that, we'd need to integrate the tracer with the system tests framework.
If we fail to extract trace context from the W3C headers, we try the next configured extraction style (Datadog by default). If no trace context can be extracted in any configured style, extraction fails. |
* update example to support new image naming scheme * working on adding example/ to CircleCI * let's make an example request in CircleCI * no bind mounts allowed * when it doubt, add more fudge * simplify the example in the CI * don't make readme files executable * shellcheck * CircleCI no likely but not say why * no need for python in the example * oops: edited the wrong job * circleci doesn't like forward slashes in job names
This implements W3C-style trace context propagation using the "traceparent" and "tracestate" headers. It is based on a internal design document.
It's an extension of work done previously in #12.
In addition to adding the
PropagationStyle::W3C, this work also resulted in the following changes:struct ExtractedDatawas factored out into its ownextracted_data.{h,cpp}.hexfunction was factored out into its ownhex.{h,cpp}.bin/checkwas added, andbin/testmodified, for minor development convenience.vector<pair<string, string>>instead of the previousunordered_map<string, string>. This made unit tests simpler and more honest, and the preservation of order is a nice property to have, likely without any performance penalty.