-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always serialize X-Datadog-Tags #242
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, and some comments/questions for consideration
src/span_context.cpp
Outdated
// Inject the trace tags, even if they're empty of if an error occurred. | ||
// This is to work around a quirk of nginx-opentracing. | ||
// See <https://github.com/DataDog/dd-opentracing-cpp/issues/241>. | ||
result = writer.Set(headers_impl.tags_header, tags ? *tags : ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge fan of ?
notation
REQUIRE(injected_json.find("tags") == injected_json.end()); | ||
const auto tags = injected_json.find("tags"); | ||
// See <https://github.com/DataDog/dd-opentracing-cpp/issues/241>. | ||
REQUIRE((tags == injected_json.end() || *tags == "")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment about why double-brackets were needed for future-me or future-you.
-H 'X-Datadog-Sampling-Priority: 0' \ | ||
http://localhost/proxy/ >/tmp/curl_log.txt | ||
|
||
if [ "$(cat ${NGINX_ERROR_LOG} | grep 'no opentracing context value found for span context key ' | wc -l)" != "0" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an easy way to verify that this captures any future propagation headers that would emit this error?
You might need to force-push a signed commit. Github basically saying "new laptop who dis?" |
- add an integration test that fails - still working on it... - always inject X-Datadog-Tags - uncomment the other integration tests - document the x-datadog-tags change - unit tests are a thing - clang-format-9 - generalize the most recent addition to the integration tests - address review comments
c8bbaf5
to
ece6059
Compare
nginx-opentracing constrains us to set non-empty values for all propagation headers we wish to register in the "hack," or else a (harmless) error message will be printed to the nginx error log.
Propagating an empty header is silly, but will do no harm. Nginx will not send it anyway, and Envoy is unlikely to be affected by these changes as we move to a new library there.