From 94b1a08e3e41293897a65b387d979a3dbcd76f7f Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 15 Nov 2023 18:19:46 -0500 Subject: [PATCH 01/12] more context in extraction error messages --- src/datadog/extracted_data.h | 8 ++++ src/datadog/tracer.cpp | 84 +++++++++++++++++++++++++++++---- src/datadog/w3c_propagation.cpp | 2 + 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/src/datadog/extracted_data.h b/src/datadog/extracted_data.h index 33b72215..9ab0271c 100644 --- a/src/datadog/extracted_data.h +++ b/src/datadog/extracted_data.h @@ -9,6 +9,7 @@ #include #include "optional.h" +#include "propagation_style.h" #include "trace_id.h" namespace datadog { @@ -33,6 +34,13 @@ struct ExtractedData { // `additional_datadog_w3c_tracestate` is null. // `additional_datadog_w3c_tracestate` is used for the `W3C` injection style. Optional additional_datadog_w3c_tracestate; + // `style` is the extraction style used to obtain this `ExtractedData`. It's + // for diagnostics. + Optional style; + // `headers_examined` are the name/value pairs of HTTP headers (or equivalent + // request meta-data) that were looked up and had values during the + // preparation of this `ExtractedData`. It's for diagnostics. + std::vector> headers_examined; }; } // namespace tracing diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 795cf6f4..96b0daf8 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -2,6 +2,7 @@ #include #include +#include #include "datadog_agent.h" #include "dict_reader.h" @@ -111,6 +112,7 @@ Expected extract_datadog( const DictReader& headers, std::unordered_map& span_tags, Logger& logger) { ExtractedData result; + result.style = PropagationStyle::DATADOG; auto trace_id = extract_id_header(headers, "x-datadog-trace-id", "trace", "Datadog", 10); @@ -160,6 +162,7 @@ Expected extract_b3( const DictReader& headers, std::unordered_map&, Logger&) { ExtractedData result; + result.style = PropagationStyle::B3; if (auto found = headers.lookup("x-b3-traceid")) { auto parsed = TraceID::parse_hex(*found); @@ -197,6 +200,59 @@ Expected extract_b3( return result; } +Expected extract_none( + const DictReader&, std::unordered_map&, Logger&) { + ExtractedData result; + result.style = PropagationStyle::NONE; + return result; +} + +std::string extraction_error_prefix( + const Optional& style, + const std::vector>& headers_examined) { + std::ostringstream stream; + stream << "While extracting trace context"; + if (style) { + stream << " in the " << to_json(*style) << " propagation style"; + } + auto it = headers_examined.begin(); + if (it != headers_examined.end()) { + stream << " from the following headers: ["; + stream << nlohmann::json(it->first + ": " + it->second); + for (++it; it != headers_examined.end(); ++it) { + stream << ", "; + stream << nlohmann::json(it->first + ": " + it->second); + } + stream << "]"; + } + stream << ", an error occurred: "; + return stream.str(); +} + +struct AuditedReader : public DictReader { + const DictReader& underlying; + mutable std::vector> entries_found; + + explicit AuditedReader(const DictReader& underlying) + : underlying(underlying) {} + + Optional lookup(StringView key) const override { + auto value = underlying.lookup(key); + if (value) { + entries_found.emplace_back(key, *value); + } + return value; + } + + void visit(const std::function& + visitor) const override { + underlying.visit([&, this](StringView key, StringView value) { + entries_found.emplace_back(key, value); + visitor(key, value); + }); + } +}; + } // namespace Tracer::Tracer(const FinalizedTracerConfig& config) @@ -302,6 +358,8 @@ Expected Tracer::extract_span(const DictReader& reader, const SpanConfig& config) { assert(!extraction_styles_.empty()); + AuditedReader audited_reader{reader}; + auto span_data = std::make_unique(); ExtractedData extracted_data; @@ -320,14 +378,16 @@ Expected Tracer::extract_span(const DictReader& reader, break; default: assert(style == PropagationStyle::NONE); - extracted_data = ExtractedData{}; - continue; + extract = &extract_none; } - auto data = extract(reader, span_data->tags, *logger_); + audited_reader.entries_found.clear(); + auto data = extract(audited_reader, span_data->tags, *logger_); if (auto* error = data.if_error()) { - return std::move(*error); + return error->with_prefix( + extraction_error_prefix(style, audited_reader.entries_found)); } extracted_data = *data; + extracted_data.headers_examined = audited_reader.entries_found; // If the extractor produced a non-null trace ID, then we consider this // extraction style the one "chosen" for this trace. // Otherwise, we loop around to the next configured extraction style. @@ -337,8 +397,8 @@ Expected Tracer::extract_span(const DictReader& reader, } auto& [trace_id, parent_id, origin, trace_tags, sampling_priority, - additional_w3c_tracestate, additional_datadog_w3c_tracestate] = - extracted_data; + additional_w3c_tracestate, additional_datadog_w3c_tracestate, style, + headers_examined] = extracted_data; // Some information might be missing. // Here are the combinations considered: @@ -357,14 +417,16 @@ Expected Tracer::extract_span(const DictReader& reader, if (!trace_id && !parent_id) { return Error{Error::NO_SPAN_TO_EXTRACT, - "There's neither a trace ID nor a parent span ID to extract."}; + "There's neither a trace ID nor a parent span ID to extract."} + .with_prefix(extraction_error_prefix(style, headers_examined)); } if (!trace_id) { std::string message; message += "There's no trace ID to extract, but there is a parent span ID: "; message += std::to_string(*parent_id); - return Error{Error::MISSING_TRACE_ID, std::move(message)}; + return Error{Error::MISSING_TRACE_ID, std::move(message)}.with_prefix( + extraction_error_prefix(style, headers_examined)); } if (!parent_id && !origin) { std::string message; @@ -377,7 +439,8 @@ Expected Tracer::extract_span(const DictReader& reader, message += std::to_string(trace_id->low); } message += ']'; - return Error{Error::MISSING_PARENT_SPAN_ID, std::move(message)}; + return Error{Error::MISSING_PARENT_SPAN_ID, std::move(message)}.with_prefix( + extraction_error_prefix(style, headers_examined)); } if (!parent_id) { @@ -392,7 +455,8 @@ Expected Tracer::extract_span(const DictReader& reader, if (*trace_id == 0) { return Error{Error::ZERO_TRACE_ID, - "extracted zero value for trace ID, which is invalid"}; + "extracted zero value for trace ID, which is invalid"} + .with_prefix(extraction_error_prefix(style, headers_examined)); } // We're done extracting fields. Now create the span. diff --git a/src/datadog/w3c_propagation.cpp b/src/datadog/w3c_propagation.cpp index 0cb1fcba..eae5cdd5 100644 --- a/src/datadog/w3c_propagation.cpp +++ b/src/datadog/w3c_propagation.cpp @@ -9,6 +9,7 @@ #include "dict_reader.h" #include "hex.h" #include "parse_util.h" +#include "propagation_style.h" #include "tags.h" namespace datadog { @@ -284,6 +285,7 @@ Expected extract_w3c( const DictReader& headers, std::unordered_map& span_tags, Logger&) { ExtractedData result; + result.style = PropagationStyle::W3C; if (auto error_tag_value = extract_traceparent(result, headers)) { span_tags[tags::internal::w3c_extraction_error] = From 351f315502ff091aac13b139692d808cbb3ac83e Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 16 Nov 2023 13:59:29 -0500 Subject: [PATCH 02/12] (unrelated) update library version in example --- examples/http-server/server/install-dd-trace-cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/http-server/server/install-dd-trace-cpp b/examples/http-server/server/install-dd-trace-cpp index e8965fb4..b3025114 100755 --- a/examples/http-server/server/install-dd-trace-cpp +++ b/examples/http-server/server/install-dd-trace-cpp @@ -5,7 +5,7 @@ set -e # Adjust for the latest release. # See . -VERSION_TAG=v0.1.9 +VERSION_TAG=v0.1.10 cd /tmp git clone --branch "$VERSION_TAG" 'https://github.com/datadog/dd-trace-cpp' From 22483271e09b9a6745551e6d08078329ccf49775 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 16 Nov 2023 14:37:34 -0500 Subject: [PATCH 03/12] move extraction code into its own component --- BUILD.bazel | 2 + CMakeLists.txt | 2 + src/datadog/extraction_util.cpp | 240 ++++++++++++++++++++++++++++++++ src/datadog/extraction_util.h | 90 ++++++++++++ src/datadog/tracer.cpp | 229 +----------------------------- 5 files changed, 335 insertions(+), 228 deletions(-) create mode 100644 src/datadog/extraction_util.cpp create mode 100644 src/datadog/extraction_util.h diff --git a/BUILD.bazel b/BUILD.bazel index 6f039007..a91d7ec6 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -11,6 +11,7 @@ cc_library( "src/datadog/default_http_client_null.cpp", "src/datadog/environment.cpp", "src/datadog/error.cpp", + "src/datadog/extraction_util.cpp", "src/datadog/glob.cpp", "src/datadog/id_generator.cpp", "src/datadog/limiter.cpp", @@ -59,6 +60,7 @@ cc_library( "src/datadog/event_scheduler.h", "src/datadog/expected.h", "src/datadog/extracted_data.h", + "src/datadog/extraction_util.h", "src/datadog/glob.h", "src/datadog/hex.h", "src/datadog/http_client.h", diff --git a/CMakeLists.txt b/CMakeLists.txt index a0336c13..3dbcd309 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -106,6 +106,7 @@ target_sources(dd_trace_cpp-objects PRIVATE # src/datadog/default_http_client_null.cpp use libcurl src/datadog/environment.cpp src/datadog/error.cpp + src/datadog/extraction_util.cpp src/datadog/glob.cpp src/datadog/id_generator.cpp src/datadog/limiter.cpp @@ -160,6 +161,7 @@ target_sources(dd_trace_cpp-objects PUBLIC src/datadog/event_scheduler.h src/datadog/expected.h src/datadog/extracted_data.h + src/datadog/extraction_util.h src/datadog/glob.h src/datadog/hex.h src/datadog/http_client.h diff --git a/src/datadog/extraction_util.cpp b/src/datadog/extraction_util.cpp new file mode 100644 index 00000000..c96913a1 --- /dev/null +++ b/src/datadog/extraction_util.cpp @@ -0,0 +1,240 @@ +#include "extraction_util.h" + +#include +#include +#include +#include + +#include "extracted_data.h" +#include "json.hpp" +#include "logger.h" +#include "parse_util.h" +#include "tag_propagation.h" +#include "tags.h" + +namespace datadog { +namespace tracing { + +// Parse the high 64 bits of a trace ID from the specified `value`. If `value` +// is correctly formatted, then return the resulting bits. If `value` is +// incorrectly formatted then return `nullopt`. +Optional parse_trace_id_high(const std::string& value) { + if (value.size() != 16) { + return nullopt; + } + + auto high = parse_uint64(value, 16); + if (high) { + return *high; + } + + return nullopt; +} + +// Decode the specified `trace_tags` and integrate them into the specified +// `result`. If an error occurs, add a `tags::internal::propagation_error` tag +// to the specified `span_tags` and log a diagnostic using the specified +// `logger`. +void handle_trace_tags(StringView trace_tags, ExtractedData& result, + std::unordered_map& span_tags, + Logger& logger) { + auto maybe_trace_tags = decode_tags(trace_tags); + if (auto* error = maybe_trace_tags.if_error()) { + logger.log_error(*error); + span_tags[tags::internal::propagation_error] = "decoding_error"; + return; + } + + for (auto& [key, value] : *maybe_trace_tags) { + if (!starts_with(key, "_dd.p.")) { + continue; + } + + if (key == tags::internal::trace_id_high) { + // _dd.p.tid contains the high 64 bits of the trace ID. + const Optional high = parse_trace_id_high(value); + if (!high) { + span_tags[tags::internal::propagation_error] = "malformed_tid " + value; + continue; + } + + if (result.trace_id) { + // Note that this assumes the lower 64 bits of the trace ID have already + // been extracted (i.e. we look for X-Datadog-Trace-ID first). + result.trace_id->high = *high; + } + } + + result.trace_tags.emplace_back(std::move(key), std::move(value)); + } +} + +Expected> extract_id_header(const DictReader& headers, + StringView header, + StringView header_kind, + StringView style_name, + int base) { + auto found = headers.lookup(header); + if (!found) { + return nullopt; + } + auto result = parse_uint64(*found, base); + if (auto* error = result.if_error()) { + std::string prefix; + prefix += "Could not extract "; + append(prefix, style_name); + prefix += "-style "; + append(prefix, header_kind); + prefix += "ID from "; + append(prefix, header); + prefix += ": "; + append(prefix, *found); + prefix += ' '; + return error->with_prefix(prefix); + } + return *result; +} + +Expected extract_datadog( + const DictReader& headers, + std::unordered_map& span_tags, Logger& logger) { + ExtractedData result; + result.style = PropagationStyle::DATADOG; + + auto trace_id = + extract_id_header(headers, "x-datadog-trace-id", "trace", "Datadog", 10); + if (auto* error = trace_id.if_error()) { + return std::move(*error); + } + if (*trace_id) { + result.trace_id = TraceID(**trace_id); + } + + auto parent_id = extract_id_header(headers, "x-datadog-parent-id", + "parent span", "Datadog", 10); + if (auto* error = parent_id.if_error()) { + return std::move(*error); + } + result.parent_id = *parent_id; + + const StringView sampling_priority_header = "x-datadog-sampling-priority"; + if (auto found = headers.lookup(sampling_priority_header)) { + auto sampling_priority = parse_int(*found, 10); + if (auto* error = sampling_priority.if_error()) { + std::string prefix; + prefix += "Could not extract Datadog-style sampling priority from "; + append(prefix, sampling_priority_header); + prefix += ": "; + append(prefix, *found); + prefix += ' '; + return error->with_prefix(prefix); + } + result.sampling_priority = *sampling_priority; + } + + auto origin = headers.lookup("x-datadog-origin"); + if (origin) { + result.origin = std::string(*origin); + } + + auto trace_tags = headers.lookup("x-datadog-tags"); + if (trace_tags) { + handle_trace_tags(*trace_tags, result, span_tags, logger); + } + + return result; +} + +Expected extract_b3( + const DictReader& headers, std::unordered_map&, + Logger&) { + ExtractedData result; + result.style = PropagationStyle::B3; + + if (auto found = headers.lookup("x-b3-traceid")) { + auto parsed = TraceID::parse_hex(*found); + if (auto* error = parsed.if_error()) { + std::string prefix = "Could not extract B3-style trace ID from \""; + append(prefix, *found); + prefix += "\": "; + return error->with_prefix(prefix); + } + result.trace_id = *parsed; + } + + auto parent_id = + extract_id_header(headers, "x-b3-spanid", "parent span", "B3", 16); + if (auto* error = parent_id.if_error()) { + return std::move(*error); + } + result.parent_id = *parent_id; + + const StringView sampling_priority_header = "x-b3-sampled"; + if (auto found = headers.lookup(sampling_priority_header)) { + auto sampling_priority = parse_int(*found, 10); + if (auto* error = sampling_priority.if_error()) { + std::string prefix; + prefix += "Could not extract B3-style sampling priority from "; + append(prefix, sampling_priority_header); + prefix += ": "; + append(prefix, *found); + prefix += ' '; + return error->with_prefix(prefix); + } + result.sampling_priority = *sampling_priority; + } + + return result; +} + +Expected extract_none( + const DictReader&, std::unordered_map&, Logger&) { + ExtractedData result; + result.style = PropagationStyle::NONE; + return result; +} + +std::string extraction_error_prefix( + const Optional& style, + const std::vector>& headers_examined) { + std::ostringstream stream; + stream << "While extracting trace context"; + if (style) { + stream << " in the " << to_json(*style) << " propagation style"; + } + auto it = headers_examined.begin(); + if (it != headers_examined.end()) { + stream << " from the following headers: ["; + stream << nlohmann::json(it->first + ": " + it->second); + for (++it; it != headers_examined.end(); ++it) { + stream << ", "; + stream << nlohmann::json(it->first + ": " + it->second); + } + stream << "]"; + } + stream << ", an error occurred: "; + return stream.str(); +} + +AuditedReader::AuditedReader(const DictReader& underlying) + : underlying(underlying) {} + +Optional AuditedReader::lookup(StringView key) const { + auto value = underlying.lookup(key); + if (value) { + entries_found.emplace_back(key, *value); + } + return value; +} + +void AuditedReader::visit( + const std::function& visitor) + const { + underlying.visit([&, this](StringView key, StringView value) { + entries_found.emplace_back(key, value); + visitor(key, value); + }); +} + +} // namespace tracing +} // namespace datadog diff --git a/src/datadog/extraction_util.h b/src/datadog/extraction_util.h new file mode 100644 index 00000000..cb148494 --- /dev/null +++ b/src/datadog/extraction_util.h @@ -0,0 +1,90 @@ +#pragma once + +// This component provides facilities for extracting trace context from a +// `DictReader`. It is used by `Tracer::extract_trace`. See `tracer.cpp`. + +#include +#include +#include +#include +#include + +#include "dict_reader.h" +#include "expected.h" +#include "optional.h" +#include "propagation_style.h" + +namespace datadog { +namespace tracing { + +struct ExtractedData; +class Logger; + +// Parse the high 64 bits of a trace ID from the specified `value`. If `value` +// is correctly formatted, then return the resulting bits. If `value` is +// incorrectly formatted then return `nullopt`. +Optional parse_trace_id_high(const std::string& value); + +// Decode the specified `trace_tags` and integrate them into the specified +// `result`. If an error occurs, add a `tags::internal::propagation_error` tag +// to the specified `span_tags` and log a diagnostic using the specified +// `logger`. +void handle_trace_tags(StringView trace_tags, ExtractedData& result, + std::unordered_map& span_tags, + Logger& logger); + +// Extract an ID from the specified `header`, which might be present in the +// specified `headers`, and return the ID. If `header` is not present in +// `headers`, then return `nullopt`. If an error occurs, return an `Error`. +// Parse the ID with respect to the specified numeric `base`, e.g. `10` or `16`. +// The specified `header_kind` and `style_name` are used in diagnostic messages +// should an error occur. +Expected> extract_id_header(const DictReader& headers, + StringView header, + StringView header_kind, + StringView style_name, + int base); + +// Return trace information parsed from the specified `headers` in the Datadog +// propagation style. Use the specified `span_tags` and `logger` to report +// warnings. If an error occurs, return an `Error`. +Expected extract_datadog( + const DictReader& headers, + std::unordered_map& span_tags, Logger& logger); + +// Return trace information parsed from the specified `headers` in the B3 +// multi-header propagation style. If an error occurs, return an `Error`. +Expected extract_b3( + const DictReader& headers, std::unordered_map&, + Logger&); + +// Return a default constructed `ExtractedData`, which indicates the absence of +// extracted trace information. +Expected extract_none( + const DictReader&, std::unordered_map&, Logger&); + +// Return a string that can be used as the argument to `Error::with_prefix` for +// errors occurring while extracting trace information in the specified `style` +// from the specified `headers_examined`. +std::string extraction_error_prefix( + const Optional& style, + const std::vector>& headers_examined); + +// `AuditedReader` is a `DictReader` that remembers all key/value pairs looked +// up or visited through it. It remembers a lookup only if it yielded a non-null +// value. This is used for error diagnostic messages in trace extraction (i.e. +// an error occurred, but which HTTP request headers were we looking at?). +struct AuditedReader : public DictReader { + const DictReader& underlying; + mutable std::vector> entries_found; + + explicit AuditedReader(const DictReader& underlying); + + Optional lookup(StringView key) const override; + + void visit(const std::function& + visitor) const override; +}; + +} // namespace tracing +} // namespace datadog diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 96b0daf8..78fb5905 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -8,6 +8,7 @@ #include "dict_reader.h" #include "environment.h" #include "extracted_data.h" +#include "extraction_util.h" #include "hex.h" #include "json.hpp" #include "logger.h" @@ -26,234 +27,6 @@ namespace datadog { namespace tracing { -namespace { - -// Parse the high 64 bits of a trace ID from the specified `value`. If `value` -// is correctly formatted, then return the resulting bits. If `value` is -// incorrectly formatted then return `nullopt`. -Optional parse_trace_id_high(const std::string& value) { - if (value.size() != 16) { - return nullopt; - } - - auto high = parse_uint64(value, 16); - if (high) { - return *high; - } - - return nullopt; -} - -// Decode the specified `trace_tags` and integrate them into the specified -// `result`. If an error occurs, add a `tags::internal::propagation_error` tag -// to the specified `span_tags` and log a diagnostic using the specified -// `logger`. -void handle_trace_tags(StringView trace_tags, ExtractedData& result, - std::unordered_map& span_tags, - Logger& logger) { - auto maybe_trace_tags = decode_tags(trace_tags); - if (auto* error = maybe_trace_tags.if_error()) { - logger.log_error(*error); - span_tags[tags::internal::propagation_error] = "decoding_error"; - return; - } - - for (auto& [key, value] : *maybe_trace_tags) { - if (!starts_with(key, "_dd.p.")) { - continue; - } - - if (key == tags::internal::trace_id_high) { - // _dd.p.tid contains the high 64 bits of the trace ID. - const Optional high = parse_trace_id_high(value); - if (!high) { - span_tags[tags::internal::propagation_error] = "malformed_tid " + value; - continue; - } - - if (result.trace_id) { - // Note that this assumes the lower 64 bits of the trace ID have already - // been extracted (i.e. we look for X-Datadog-Trace-ID first). - result.trace_id->high = *high; - } - } - - result.trace_tags.emplace_back(std::move(key), std::move(value)); - } -} - -Expected> extract_id_header(const DictReader& headers, - StringView header, - StringView header_kind, - StringView style_name, - int base) { - auto found = headers.lookup(header); - if (!found) { - return nullopt; - } - auto result = parse_uint64(*found, base); - if (auto* error = result.if_error()) { - std::string prefix; - prefix += "Could not extract "; - append(prefix, style_name); - prefix += "-style "; - append(prefix, header_kind); - prefix += "ID from "; - append(prefix, header); - prefix += ": "; - append(prefix, *found); - prefix += ' '; - return error->with_prefix(prefix); - } - return *result; -} - -Expected extract_datadog( - const DictReader& headers, - std::unordered_map& span_tags, Logger& logger) { - ExtractedData result; - result.style = PropagationStyle::DATADOG; - - auto trace_id = - extract_id_header(headers, "x-datadog-trace-id", "trace", "Datadog", 10); - if (auto* error = trace_id.if_error()) { - return std::move(*error); - } - if (*trace_id) { - result.trace_id = TraceID(**trace_id); - } - - auto parent_id = extract_id_header(headers, "x-datadog-parent-id", - "parent span", "Datadog", 10); - if (auto* error = parent_id.if_error()) { - return std::move(*error); - } - result.parent_id = *parent_id; - - const StringView sampling_priority_header = "x-datadog-sampling-priority"; - if (auto found = headers.lookup(sampling_priority_header)) { - auto sampling_priority = parse_int(*found, 10); - if (auto* error = sampling_priority.if_error()) { - std::string prefix; - prefix += "Could not extract Datadog-style sampling priority from "; - append(prefix, sampling_priority_header); - prefix += ": "; - append(prefix, *found); - prefix += ' '; - return error->with_prefix(prefix); - } - result.sampling_priority = *sampling_priority; - } - - auto origin = headers.lookup("x-datadog-origin"); - if (origin) { - result.origin = std::string(*origin); - } - - auto trace_tags = headers.lookup("x-datadog-tags"); - if (trace_tags) { - handle_trace_tags(*trace_tags, result, span_tags, logger); - } - - return result; -} - -Expected extract_b3( - const DictReader& headers, std::unordered_map&, - Logger&) { - ExtractedData result; - result.style = PropagationStyle::B3; - - if (auto found = headers.lookup("x-b3-traceid")) { - auto parsed = TraceID::parse_hex(*found); - if (auto* error = parsed.if_error()) { - std::string prefix = "Could not extract B3-style trace ID from \""; - append(prefix, *found); - prefix += "\": "; - return error->with_prefix(prefix); - } - result.trace_id = *parsed; - } - - auto parent_id = - extract_id_header(headers, "x-b3-spanid", "parent span", "B3", 16); - if (auto* error = parent_id.if_error()) { - return std::move(*error); - } - result.parent_id = *parent_id; - - const StringView sampling_priority_header = "x-b3-sampled"; - if (auto found = headers.lookup(sampling_priority_header)) { - auto sampling_priority = parse_int(*found, 10); - if (auto* error = sampling_priority.if_error()) { - std::string prefix; - prefix += "Could not extract B3-style sampling priority from "; - append(prefix, sampling_priority_header); - prefix += ": "; - append(prefix, *found); - prefix += ' '; - return error->with_prefix(prefix); - } - result.sampling_priority = *sampling_priority; - } - - return result; -} - -Expected extract_none( - const DictReader&, std::unordered_map&, Logger&) { - ExtractedData result; - result.style = PropagationStyle::NONE; - return result; -} - -std::string extraction_error_prefix( - const Optional& style, - const std::vector>& headers_examined) { - std::ostringstream stream; - stream << "While extracting trace context"; - if (style) { - stream << " in the " << to_json(*style) << " propagation style"; - } - auto it = headers_examined.begin(); - if (it != headers_examined.end()) { - stream << " from the following headers: ["; - stream << nlohmann::json(it->first + ": " + it->second); - for (++it; it != headers_examined.end(); ++it) { - stream << ", "; - stream << nlohmann::json(it->first + ": " + it->second); - } - stream << "]"; - } - stream << ", an error occurred: "; - return stream.str(); -} - -struct AuditedReader : public DictReader { - const DictReader& underlying; - mutable std::vector> entries_found; - - explicit AuditedReader(const DictReader& underlying) - : underlying(underlying) {} - - Optional lookup(StringView key) const override { - auto value = underlying.lookup(key); - if (value) { - entries_found.emplace_back(key, *value); - } - return value; - } - - void visit(const std::function& - visitor) const override { - underlying.visit([&, this](StringView key, StringView value) { - entries_found.emplace_back(key, value); - visitor(key, value); - }); - } -}; - -} // namespace Tracer::Tracer(const FinalizedTracerConfig& config) : Tracer(config, default_id_generator(config.trace_id_128_bit)) {} From 92516021e12d915dd8e9489f33c8d14ee3ffff72 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 16 Nov 2023 14:42:45 -0500 Subject: [PATCH 04/12] remove copy/pasted comments --- src/datadog/extraction_util.cpp | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/datadog/extraction_util.cpp b/src/datadog/extraction_util.cpp index c96913a1..8f2d22f4 100644 --- a/src/datadog/extraction_util.cpp +++ b/src/datadog/extraction_util.cpp @@ -15,9 +15,6 @@ namespace datadog { namespace tracing { -// Parse the high 64 bits of a trace ID from the specified `value`. If `value` -// is correctly formatted, then return the resulting bits. If `value` is -// incorrectly formatted then return `nullopt`. Optional parse_trace_id_high(const std::string& value) { if (value.size() != 16) { return nullopt; @@ -31,10 +28,6 @@ Optional parse_trace_id_high(const std::string& value) { return nullopt; } -// Decode the specified `trace_tags` and integrate them into the specified -// `result`. If an error occurs, add a `tags::internal::propagation_error` tag -// to the specified `span_tags` and log a diagnostic using the specified -// `logger`. void handle_trace_tags(StringView trace_tags, ExtractedData& result, std::unordered_map& span_tags, Logger& logger) { From 7a625cc532f79ed68955da31afb005819f6c1b42 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 16 Nov 2023 14:45:08 -0500 Subject: [PATCH 05/12] eats, shoots, and leaves --- src/datadog/extraction_util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/datadog/extraction_util.h b/src/datadog/extraction_util.h index cb148494..e46c2f14 100644 --- a/src/datadog/extraction_util.h +++ b/src/datadog/extraction_util.h @@ -22,7 +22,7 @@ class Logger; // Parse the high 64 bits of a trace ID from the specified `value`. If `value` // is correctly formatted, then return the resulting bits. If `value` is -// incorrectly formatted then return `nullopt`. +// incorrectly formatted, then return `nullopt`. Optional parse_trace_id_high(const std::string& value); // Decode the specified `trace_tags` and integrate them into the specified From 1a1bd9da5405d832a5b25d599a24ce39db23b5f1 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 16 Nov 2023 15:03:22 -0500 Subject: [PATCH 06/12] bin/format should forward arguments to clang-format This is used by bin/check to do a "dry run" format. --- bin/format | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bin/format b/bin/format index e0ed77fb..c1c6a2ca 100755 --- a/bin/format +++ b/bin/format @@ -11,7 +11,7 @@ cd "$(dirname "$0")"/.. # occasionally bumps the required version, reformatting everything. version=14 formatter=clang-format-$version -formatter_options="--style=file -i" +formatter_options="--style=file -i $*" find_sources() { find src/ examples/ test/ fuzz/ -type f \( -name '*.h' -o -name '*.cpp' \) "$@" From a0b50dbf1b9897b40c94b187a94cf164868d107b Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 16 Nov 2023 15:04:04 -0500 Subject: [PATCH 07/12] default propagation style is now [Datadog, W3C] --- src/datadog/tracer_config.h | 6 ++++-- test/test_tracer_config.cpp | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/datadog/tracer_config.h b/src/datadog/tracer_config.h index 77a8cebb..b59ad0c3 100644 --- a/src/datadog/tracer_config.h +++ b/src/datadog/tracer_config.h @@ -72,7 +72,8 @@ struct TracerConfig { // All styles indicated by `injection_styles` are used for injection. // `injection_styles` is overridden by the `DD_TRACE_PROPAGATION_STYLE_INJECT` // and `DD_TRACE_PROPAGATION_STYLE` environment variables. - std::vector injection_styles = {PropagationStyle::DATADOG}; + std::vector injection_styles = {PropagationStyle::DATADOG, + PropagationStyle::W3C}; // `extraction_styles` indicates with which tracing systems trace propagation // will be compatible when extracting (receiving) trace context. @@ -82,7 +83,8 @@ struct TracerConfig { // `extraction_styles` is overridden by the // `DD_TRACE_PROPAGATION_STYLE_EXTRACT` and `DD_TRACE_PROPAGATION_STYLE` // environment variables. - std::vector extraction_styles = {PropagationStyle::DATADOG}; + std::vector extraction_styles = {PropagationStyle::DATADOG, + PropagationStyle::W3C}; // `report_hostname` indicates whether the tracer will include the result of // `gethostname` with traces sent to the collector. diff --git a/test/test_tracer_config.cpp b/test/test_tracer_config.cpp index 382f9c2e..0cd6e7fb 100644 --- a/test/test_tracer_config.cpp +++ b/test/test_tracer_config.cpp @@ -1003,12 +1003,12 @@ TEST_CASE("TracerConfig propagation styles") { TracerConfig config; config.defaults.service = "testsvc"; - SECTION("default style is Datadog") { + SECTION("default style is [Datadog, W3C]") { auto finalized = finalize_config(config); REQUIRE(finalized); const std::vector expected_styles = { - PropagationStyle::DATADOG}; + PropagationStyle::DATADOG, PropagationStyle::W3C}; REQUIRE(finalized->injection_styles == expected_styles); REQUIRE(finalized->extraction_styles == expected_styles); From ec3f06eccf5300c0d99421afb6342dc0b7a4433b Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 16 Nov 2023 15:45:26 -0500 Subject: [PATCH 08/12] heterogeneous extraction, untested --- src/datadog/extraction_util.cpp | 50 +++++++++++++++++++++++++++++++++ src/datadog/extraction_util.h | 7 +++++ src/datadog/tracer.cpp | 18 ++++-------- 3 files changed, 63 insertions(+), 12 deletions(-) diff --git a/src/datadog/extraction_util.cpp b/src/datadog/extraction_util.cpp index 8f2d22f4..d4b27b81 100644 --- a/src/datadog/extraction_util.cpp +++ b/src/datadog/extraction_util.cpp @@ -1,5 +1,6 @@ #include "extraction_util.h" +#include #include #include #include @@ -229,5 +230,54 @@ void AuditedReader::visit( }); } +ExtractedData merge(const std::vector& contexts) { + ExtractedData result; + + const auto found = std::find_if( + contexts.begin(), contexts.end(), + [](const ExtractedData& data) { return data.trace_id.has_value(); }); + + if (found == contexts.end()) { + // Nothing extracted a trace ID. Return the first context that includes a + // parent ID, if any, or otherwise just return an empty `ExtractedData`. + // The purpose of looking for a parent ID is to allow for the error + // "extracted a parent ID without a trace ID," if that's what happened. + const auto other = std::find_if( + contexts.begin(), contexts.end(), + [](const ExtractedData& data) { return data.parent_id.has_value(); }); + if (other != contexts.end()) { + result = *other; + } + return result; + } + + // `found` refers to the first extracted context that yielded a trace ID. + // This will be our main context. + // + // If the style of `found` is not W3C, then examine the remaining contexts + // for W3C-style tracestate that we might want to include in `result`. + result = *found; + if (result.style == PropagationStyle::W3C) { + return result; + } + + const auto other = + std::find_if(found, contexts.end(), [&](const ExtractedData& data) { + return data.style == PropagationStyle::W3C && + data.trace_id == found->trace_id; + }); + + if (other != contexts.end()) { + result.additional_w3c_tracestate = other->additional_w3c_tracestate; + result.additional_datadog_w3c_tracestate = + other->additional_datadog_w3c_tracestate; + result.headers_examined.insert(result.headers_examined.end(), + other->headers_examined.begin(), + other->headers_examined.end()); + } + + return result; +} + } // namespace tracing } // namespace datadog diff --git a/src/datadog/extraction_util.h b/src/datadog/extraction_util.h index e46c2f14..c16fce0f 100644 --- a/src/datadog/extraction_util.h +++ b/src/datadog/extraction_util.h @@ -86,5 +86,12 @@ struct AuditedReader : public DictReader { visitor) const override; }; +// Combine the specified trace `contexts`, each of which was extracted in a +// particular propagation style, into one `ExtractedData` that includes fields +// from compatible elements of `contexts`, and return the resulting +// `ExtractedData`. The order of the elements of `contexts` must correspond to +// the order of the configured extraction propagation styles. +ExtractedData merge(const std::vector& contexts); + } // namespace tracing } // namespace datadog diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 78fb5905..272be473 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -134,7 +134,7 @@ Expected Tracer::extract_span(const DictReader& reader, AuditedReader audited_reader{reader}; auto span_data = std::make_unique(); - ExtractedData extracted_data; + std::vector extracted_contexts; for (const auto style : extraction_styles_) { using Extractor = decltype(&extract_datadog); // function pointer @@ -159,19 +159,13 @@ Expected Tracer::extract_span(const DictReader& reader, return error->with_prefix( extraction_error_prefix(style, audited_reader.entries_found)); } - extracted_data = *data; - extracted_data.headers_examined = audited_reader.entries_found; - // If the extractor produced a non-null trace ID, then we consider this - // extraction style the one "chosen" for this trace. - // Otherwise, we loop around to the next configured extraction style. - if (extracted_data.trace_id) { - break; - } + extracted_contexts.push_back(std::move(*data)); + extracted_contexts.back().headers_examined = audited_reader.entries_found; } - auto& [trace_id, parent_id, origin, trace_tags, sampling_priority, - additional_w3c_tracestate, additional_datadog_w3c_tracestate, style, - headers_examined] = extracted_data; + auto [trace_id, parent_id, origin, trace_tags, sampling_priority, + additional_w3c_tracestate, additional_datadog_w3c_tracestate, style, + headers_examined] = merge(extracted_contexts); // Some information might be missing. // Here are the combinations considered: From fa833952a3f5edcbb4de14e7d9c0686977ab0ad9 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 16 Nov 2023 20:21:23 -0500 Subject: [PATCH 09/12] unit test for heterogeneous extraction --- test/test_tracer.cpp | 114 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 3654c85a..959c2b96 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include "matchers.h" #include "mocks/collectors.h" @@ -1175,3 +1176,116 @@ TEST_CASE( REQUIRE(found->second == test_case.expected_error_prefix + test_case.tid_tag_value); } + +TEST_CASE("heterogeneous extraction") { + // These test cases verify that when W3C is among the configured extraction + // styles, then non-Datadog and unexpected Datadog fields in an incoming + // `tracestate` are extracted, under certain conditions, even when trace + // context was extracted in a non-W3C style. + // + // The idea is that a tracer might be configured to extract, e.g., + // [DATADOG, B3, W3C] and to inject [DATADOG, W3C]. We want to make + // sure that no W3C-relevant information from the incoming request is lost in + // the outgoing W3C headers, even if trace context is extracted on account of + // DATADOG or B3. + // + // See the `TestCase` instances, below, for more information. + + class MockIDGenerator : public IDGenerator { + public: + TraceID trace_id(const TimePoint&) const override { + throw std::logic_error("This test should not generate a trace ID."); + } + std::uint64_t span_id() const override { return 0x2a; } + }; + + struct TestCase { + int line; + std::string description; + std::vector extraction_styles; + std::vector injection_styles; + std::unordered_map extracted_headers; + std::unordered_map expected_injected_headers; + }; + + // clang-format off + auto test_case = GENERATE(values({ + {__LINE__, "tracestate from primary style", + {PropagationStyle::W3C, PropagationStyle::DATADOG}, + {PropagationStyle::W3C}, + {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01"}, + {"tracestate", "dd=foo:bar,lol=wut"}}, + {{"traceparent", "00-4bf92f3577b34da6a3ce929d0e0e4736-000000000000002a-01"}, + {"tracestate", "dd=s:1;foo:bar,lol=wut"}}}, + + {__LINE__, "tracestate from subsequent style", + {PropagationStyle::DATADOG, PropagationStyle::W3C}, + {PropagationStyle::W3C}, + {{"x-datadog-trace-id", "48"}, {"x-datadog-parent-id", "64"}, + {"x-datadog-origin", "Kansas"}, {"x-datadog-sampling-priority", "2"}, + {"traceparent", "00-00000000000000000000000000000030-0000000000000040-01"}, + {"tracestate", "competitor=stuff,dd=o:Nebraska;s:1;ah:choo"}}, // origin is different + {{"traceparent", "00-00000000000000000000000000000030-000000000000002a-01"}, + {"tracestate", "dd=s:2;o:Kansas;ah:choo,competitor=stuff"}}}, + + {__LINE__, "ignore interlopers", + {PropagationStyle::DATADOG, PropagationStyle::B3, PropagationStyle::W3C}, + {PropagationStyle::W3C}, + {{"x-datadog-trace-id", "48"}, {"x-datadog-parent-id", "64"}, + {"x-datadog-origin", "Kansas"}, {"x-datadog-sampling-priority", "2"}, + {"x-b3-traceid", "00000000000000000000000000000030"}, + {"x-b3-parentspanid", "000000000000002a"}, + {"x-b3-sampled", "0"}, // sampling is different + {"traceparent", "00-00000000000000000000000000000030-0000000000000040-01"}, + {"tracestate", "competitor=stuff,dd=o:Nebraska;s:1;ah:choo"}}, + {{"traceparent", "00-00000000000000000000000000000030-000000000000002a-01"}, + {"tracestate", "dd=s:2;o:Kansas;ah:choo,competitor=stuff"}}}, + + {__LINE__, "don't take tracestate if trace ID doesn't match", + {PropagationStyle::DATADOG, PropagationStyle::W3C}, + {PropagationStyle::W3C}, + {{"x-datadog-trace-id", "48"}, {"x-datadog-parent-id", "64"}, + {"x-datadog-origin", "Kansas"}, {"x-datadog-sampling-priority", "2"}, + {"traceparent", "00-00000000000000000000000000000031-0000000000000040-01"}, + {"tracestate", "competitor=stuff,dd=o:Nebraska;s:1;ah:choo"}}, + {{"traceparent", "00-00000000000000000000000000000030-000000000000002a-01"}, + {"tracestate", "dd=s:2;o:Kansas"}}}, + + {__LINE__, "don't take tracestate if W3C extraction isn't configured", + {PropagationStyle::DATADOG, PropagationStyle::B3}, + {PropagationStyle::W3C}, + {{"x-datadog-trace-id", "48"}, {"x-datadog-parent-id", "64"}, + {"x-datadog-origin", "Kansas"}, {"x-datadog-sampling-priority", "2"}, + {"traceparent", "00-00000000000000000000000000000030-0000000000000040-01"}, + {"tracestate", "competitor=stuff,dd=o:Nebraska;s:1;ah:choo"}}, + {{"traceparent", "00-00000000000000000000000000000030-000000000000002a-01"}, + {"tracestate", "dd=s:2;o:Kansas"}}}, + })); + // clang-format on + + CAPTURE(test_case.line); + CAPTURE(test_case.description); + CAPTURE(to_json(test_case.extraction_styles)); + CAPTURE(to_json(test_case.injection_styles)); + CAPTURE(test_case.extracted_headers); + CAPTURE(test_case.expected_injected_headers); + + TracerConfig config; + config.defaults.service = "testsvc"; + config.extraction_styles = test_case.extraction_styles; + config.injection_styles = test_case.injection_styles; + config.logger = std::make_shared(); + + auto finalized_config = finalize_config(config); + REQUIRE(finalized_config); + Tracer tracer{*finalized_config, std::make_shared()}; + + MockDictReader reader{test_case.extracted_headers}; + auto span = tracer.extract_span(reader); + REQUIRE(span); + + MockDictWriter writer; + span->inject(writer); + + REQUIRE(writer.items == test_case.expected_injected_headers); +} From 95bb86d9248431b27b32bcfbe7e30ae71def7d67 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 17 Nov 2023 14:52:35 -0500 Subject: [PATCH 10/12] missed a spot when moving code into extraction_util --- src/datadog/tracer.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 272be473..b43e95dd 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -2,7 +2,6 @@ #include #include -#include #include "datadog_agent.h" #include "dict_reader.h" From d7ea3b607876888b33aeca7e2864159bff976357 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 17 Nov 2023 14:55:18 -0500 Subject: [PATCH 11/12] don't lie to your teammates --- src/datadog/extraction_util.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/datadog/extraction_util.h b/src/datadog/extraction_util.h index c16fce0f..b92f10ce 100644 --- a/src/datadog/extraction_util.h +++ b/src/datadog/extraction_util.h @@ -58,8 +58,8 @@ Expected extract_b3( const DictReader& headers, std::unordered_map&, Logger&); -// Return a default constructed `ExtractedData`, which indicates the absence of -// extracted trace information. +// Return an `ExtractedData` whose only non-default field is +// `style = PropagationStyle::NONE`. Expected extract_none( const DictReader&, std::unordered_map&, Logger&); From e69b0e88d45fa52ff340a71f214d04a2b0146aa4 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 17 Nov 2023 14:58:31 -0500 Subject: [PATCH 12/12] trade a conditional jump for an addition --- src/datadog/extraction_util.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/datadog/extraction_util.cpp b/src/datadog/extraction_util.cpp index d4b27b81..3ef5fef5 100644 --- a/src/datadog/extraction_util.cpp +++ b/src/datadog/extraction_util.cpp @@ -262,7 +262,7 @@ ExtractedData merge(const std::vector& contexts) { } const auto other = - std::find_if(found, contexts.end(), [&](const ExtractedData& data) { + std::find_if(found + 1, contexts.end(), [&](const ExtractedData& data) { return data.style == PropagationStyle::W3C && data.trace_id == found->trace_id; });