Skip to content

ddtrace/tracer: update origin substitution in composeTracestate #1694

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

Merged
merged 7 commits into from
Feb 1, 2023

Conversation

lievan
Copy link
Contributor

@lievan lievan commented Jan 25, 2023

What does this PR do?

This PR updates the substitution of invalid characters in origin-value for trace state. Previously, the invalid character = was being substituted with _, which was incorrect and caused a case in test_headers_tracestate_dd.py to fail in the parametric tests. This PR updates = to be substituted with ~ before the rest of the invalid characters are replaced with _, which causes the parametric test case to pass.

Motivation

Update the substitution of invalid characters in origin to match the correct behavior W3C trace context propagation and fix a failing test case in parametric tests.

Describe how to test/QA your changes

Checkout to shevchenko/w3c-golang in the system-tests repo, update the dd-trace-go module in apps/golang to this PR, and run trace_headers tests

Reviewer's Checklist

  • If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

Sorry, something went wrong.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@lievan lievan added this to the v1.48.0 milestone Jan 25, 2023
@lievan lievan requested a review from a team January 25, 2023 22:28
Copy link
Contributor

@dianashevchenko dianashevchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ~ will be unescaped into = during extraction, we must first replace all the invalid characters including tilde with _ and then replace = with ~

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@pr-commenter
Copy link

pr-commenter bot commented Jan 26, 2023

Benchmarks

Comparing candidate commit f8e4695 in PR branch evan.li/update-origin-sub with baseline commit 7f6f95d in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
lievan and others added 3 commits January 31, 2023 09:25

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com>

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@dianashevchenko dianashevchenko merged commit 4174939 into main Feb 1, 2023
@dianashevchenko dianashevchenko deleted the evan.li/update-origin-sub branch February 1, 2023 15:50
dianashevchenko pushed a commit that referenced this pull request Feb 9, 2023

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants