chore(w3c): replace ~ with _ and = with ~ (#4822) [backport #4822 to 1.7] (#4871)

## Description
Upon review we found that `:` should be allowed in the tags value:
https://docs.datadoghq.com/getting_started/tagging/#define-tags

We were using `:` as a way to represent an "encoded" = in the propagated
trace tag values, so we decided to change the "encoded" value of the `=`
to `~`.

Therefore, we should allow `=` in tag values. In order to do this in
tracestate, we need to encode it. Therefore we encode `~`s in the value
as `_` first, and then encode `=` as `~`.

## Checklist
- [x] Followed the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
when writing a release note.
- [ ] Add additional sections for `feat` and `fix` pull requests.
- [ ] [Library
documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs)
and/or [Datadog's documentation
site](https://github.com/DataDog/documentation/) is updated. Link to doc
PR in description.

<!-- Copy and paste the relevant snippet based on the type of pull
request -->



## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added and follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines),
or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.

Co-authored-by: Munir Abdinur <[email protected]>

## Description
<!-- Briefly describe the change and why it was required. -->

<!-- If this is a breaking change, explain why it is necessary. Breaking
changes must append `!` after the type/scope. See
https://ddtrace.readthedocs.io/en/stable/contributing.html for more
details. -->

## Checklist
- [ ] Followed the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines)
when writing a release note.
- [ ] Add additional sections for `feat` and `fix` pull requests.
- [ ] [Library
documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs)
and/or [Datadog's documentation
site](https://github.com/DataDog/documentation/) is updated. Link to doc
PR in description.

<!-- Copy and paste the relevant snippet based on the type of pull
request -->

<!-- START feat -->

## Motivation
<!-- Expand on why the change is required, include relevant context for
reviewers -->

## Design 
<!-- Include benefits from the change as well as possible drawbacks and
trade-offs -->

## Testing strategy
<!-- Describe the automated tests and/or the steps for manual testing.

<!-- END feat -->

<!-- START fix -->

## Relevant issue(s)
<!-- Link the pull request to any issues related to the fix. Use
keywords for links to automate closing the issues once the pull request
is merged. -->

## Testing strategy
<!-- Describe any added regression tests and/or the manual testing
performed. -->

<!-- END fix -->

## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] Avoid breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is
included in the code or PR.
- [ ] Release note has been added and follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines),
or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.

Co-authored-by: Munir Abdinur <[email protected]>

Author: ZStriker19

ddtrace/internal/utils/http.py

@@ -6,6 +6,7 @@
 from typing import ContextManager
 from typing import Generator
 from typing import Optional
+from typing import Pattern
 from typing import Tuple
 from typing import Union
 
@@ -19,7 +20,8 @@
 from ddtrace.internal.utils.cache import cached
 
 
-_W3C_TRACESTATE_INVALID_CHARS_REGEX = r",|;|:|[^\x20-\x7E]+"
+_W3C_TRACESTATE_INVALID_CHARS_REGEX_VALUE = re.compile(r",|;|~|[^\x20-\x7E]+")
+_W3C_TRACESTATE_INVALID_CHARS_REGEX_KEY = re.compile(r",| |=|[^\x20-\x7E]+")
 
 
 Connector = Callable[[], ContextManager[compat.httplib.HTTPConnection]]
@@ -157,15 +159,22 @@ def w3c_get_dd_list_member(context):
     if context.sampling_priority is not None:
         tags.append("{}:{}".format(W3C_TRACESTATE_SAMPLING_PRIORITY_KEY, context.sampling_priority))
     if context.dd_origin:
-        # the origin value has specific values that are allowed.
-        tags.append("{}:{}".format(W3C_TRACESTATE_ORIGIN_KEY, re.sub(r",|;|=|[^\x20-\x7E]+", "_", context.dd_origin)))
+        tags.append(
+            "{}:{}".format(
+                W3C_TRACESTATE_ORIGIN_KEY,
+                w3c_encode_tag((_W3C_TRACESTATE_INVALID_CHARS_REGEX_VALUE, "_", context.dd_origin)),
+            )
+        )
+
     sampling_decision = context._meta.get(SAMPLING_DECISION_TRACE_TAG_KEY)
     if sampling_decision:
-        tags.append("t.dm:{}".format(re.sub(_W3C_TRACESTATE_INVALID_CHARS_REGEX, "_", sampling_decision)))
+        tags.append(
+            "t.dm:{}".format((w3c_encode_tag((_W3C_TRACESTATE_INVALID_CHARS_REGEX_VALUE, "_", sampling_decision))))
+        )
     # since this can change, we need to grab the value off the current span
-    usr_id_key = context._meta.get(USER_ID_KEY)
-    if usr_id_key:
-        tags.append("t.usr.id:{}".format(re.sub(_W3C_TRACESTATE_INVALID_CHARS_REGEX, "_", usr_id_key)))
+    usr_id = context._meta.get(USER_ID_KEY)
+    if usr_id:
+        tags.append("t.usr.id:{}".format(w3c_encode_tag((_W3C_TRACESTATE_INVALID_CHARS_REGEX_VALUE, "_", usr_id))))
 
     current_tags_len = sum(len(i) for i in tags)
     for k, v in context._meta.items():
@@ -176,10 +185,11 @@ def w3c_get_dd_list_member(context):
             and k not in [SAMPLING_DECISION_TRACE_TAG_KEY, USER_ID_KEY]
         ):
             # for key replace ",", "=", and characters outside the ASCII range 0x20 to 0x7E
-            # for value replace ",", ";", ":" and characters outside the ASCII range 0x20 to 0x7E
+            # for value replace ",", ";", "~" and characters outside the ASCII range 0x20 to 0x7E
+            k = k.replace("_dd.p.", "t.")
             next_tag = "{}:{}".format(
-                re.sub("_dd.p.", "t.", re.sub(r",| |=|[^\x20-\x7E]+", "_", k)),
-                re.sub(_W3C_TRACESTATE_INVALID_CHARS_REGEX, "_", v),
+                w3c_encode_tag((_W3C_TRACESTATE_INVALID_CHARS_REGEX_KEY, "_", k)),
+                w3c_encode_tag((_W3C_TRACESTATE_INVALID_CHARS_REGEX_VALUE, "_", v)),
             )
             # we need to keep the total length under 256 char
             potential_current_tags_len = current_tags_len + len(next_tag)
@@ -190,3 +200,12 @@ def w3c_get_dd_list_member(context):
                 log.debug("tracestate would exceed 256 char limit with tag: %s. Tag will not be added.", next_tag)
 
     return ";".join(tags)
+
+
+@cached()
+def w3c_encode_tag(args):
+    # type: (Tuple[Pattern, str, str]) -> str
+    pattern, replacement, tag_val = args
+    tag_val = pattern.sub(replacement, tag_val)
+    # replace = with ~ if it wasn't already replaced by the regex
+    return tag_val.replace("=", "~")

ddtrace/propagation/http.py

@@ -560,6 +560,11 @@ class _TraceContext:
         ``_dd.p.`` prefix = ``t.``
     """
 
+    @staticmethod
+    def decode_tag_val(tag_val):
+        # type str -> str
+        return tag_val.replace("~", "=")
+
     @staticmethod
     def _get_traceparent_values(tp):
         # type: (str) -> Tuple[int, int, int]
@@ -616,7 +621,8 @@ def _get_tracestate_values(ts):
             if list_mem.startswith("dd="):
                 # cut out dd= before turning into dict
                 list_mem = list_mem[3:]
-                dd = dict(item.split(":") for item in list_mem.split(";"))
+                # since tags can have a value with a :, we need to only split on the first instance of :
+                dd = dict(item.split(":", 1) for item in list_mem.split(";"))
 
         # parse out values
         if dd:
@@ -627,8 +633,13 @@ def _get_tracestate_values(ts):
                 sampling_priority_ts_int = None
 
             origin = dd.get("o")
+            if origin:
+                # we encode "=" as "~" in tracestate so need to decode here
+                origin = _TraceContext.decode_tag_val(origin)
             # need to convert from t. to _dd.p.
-            other_propagated_tags = {"_dd.p.%s" % k[2:]: v for (k, v) in dd.items() if (k.startswith("t."))}
+            other_propagated_tags = {
+                "_dd.p.%s" % k[2:]: _TraceContext.decode_tag_val(v) for (k, v) in dd.items() if k.startswith("t.")
+            }
 
             return sampling_priority_ts_int, other_propagated_tags, origin
         else:

tests/tracer/test_context.py

@@ -267,14 +267,14 @@ def test_traceparent(context, expected_traceparent):
             Context(),
             "",
         ),
-        (  # for value replace ",", ";", ":" and characters outside the ASCII range 0x20 to 0x7E with _
+        (  # for value replace ",", ";" and characters outside the ASCII range 0x20 to 0x7E with _
             Context(
                 trace_id=11803532876627986230,
                 span_id=67667974448284343,
                 sampling_priority=1,
                 meta={
                     "tracestate": "dd=s:1;o:rum;t.dm:-4;t.usr.id:baz64",
-                    "_dd.p.dm": ";5:",
+                    "_dd.p.dm": ";5;",
                     "_dd.p.usr.id": "b,z64,",
                     "_dd.p.unk": ";2",
                 },
@@ -307,7 +307,8 @@ def test_traceparent(context, expected_traceparent):
                 },
                 dd_origin=";r,um=",
             ),
-            "dd=s:1;o:_r_um_",
+            # = is encoded as ~
+            "dd=s:1;o:_r_um~",
         ),
     ],
     ids=[

tests/tracer/test_propagation.py

@@ -650,11 +650,39 @@ def test_extract_traceparent(caplog, headers, expected_tuple, expected_logging,
             ["received invalid dd header value in tracestate: 'dd=invalid,congo=123'"],
             ValueError,
         ),
-        (
+        (  # "ts_string,expected_tuple,expected_logging,expected_exception",
             "dd=foo|bar:hi|l¢¢¢¢¢¢:",
+            (None, {}, None),
+            None,
+            None,
+        ),
+        (
+            "dd=s:2;o:rum;t.dm:-4;t.usr.id:baz6~~~4",
+            # sampling_priority_ts, other_propagated_tags, origin
+            (
+                2,
+                {
+                    "_dd.p.dm": "-4",
+                    "_dd.p.usr.id": "baz6===4",
+                },
+                "rum",
+            ),
+            None,
+            None,
+        ),
+        (
+            "dd=s:2;o:rum;t.dm:-4;t.usr.id:baz:6:4",
+            # sampling_priority_ts, other_propagated_tags, origin
+            (
+                2,
+                {
+                    "_dd.p.dm": "-4",
+                    "_dd.p.usr.id": "baz:6:4",
+                },
+                "rum",
+            ),
+            None,
             None,
-            ["received invalid tracestate header: 'dd=foo|bar:hi|l¢¢¢¢¢¢:"],
-            ValueError,
         ),
     ],
     ids=[
@@ -669,6 +697,8 @@ def test_extract_traceparent(caplog, headers, expected_tuple, expected_logging,
         "tracestate_no_origin",
         "tracestate_invalid_dd_list_member",
         "tracestate_invalid_tracestate_char_outside_ascii_range_20-70",
+        "tracestate_tilda_replaced_with_equals",
+        "tracestate_colon_acceptable_char_in_value",
     ],
 )
 def test_extract_tracestate(caplog, ts_string, expected_tuple, expected_logging, expected_exception):

tests/tracer/test_utils.py

@@ -457,7 +457,7 @@ def _():
             ["s:2", "o:synthetics", "t.unknown:baz64"],
         ),
         (  # for key replace ",", "=", and characters outside the ASCII range 0x20 to 0x7E with _
-            # for value replace ",", ";", ":" and characters outside the ASCII range 0x20 to 0x7E with _
+            # for value replace ",", ";", and characters outside the ASCII range 0x20 to 0x7E with _
             Context(
                 trace_id=1234,
                 sampling_priority=2,
@@ -466,10 +466,11 @@ def _():
                     "_dd.p.unk": "-4",
                     "_dd.p.unknown": "baz64",
                     "_dd.p.¢": ";4",
-                    "_dd.p.u=,": "b:,¢a",
+                    # colons are allowed in tag values
+                    "_dd.p.u¢,": "b:,¢a",
                 },
             ),
-            ["s:2", "o:synthetics", "t.unk:-4", "t.unknown:baz64", "t._:_4", "t.u__:b___a"],
+            ["s:2", "o:synthetics", "t.unk:-4", "t.unknown:baz64", "t._:_4", "t.u__:b:__a"],
         ),
         (
             Context(
@@ -483,6 +484,18 @@ def _():
             ),
             ["s:0", "o:synthetics", "t.unk:-4", "t.unknown:baz64"],
         ),
+        (
+            Context(
+                trace_id=1234,
+                sampling_priority=2,
+                dd_origin="syn=",
+                meta={
+                    "_dd.p.unk": "-4~",
+                    "_dd.p.unknown": "baz64",
+                },
+            ),
+            ["s:2", "o:syn~", "t.unk:-4_", "t.unknown:baz64"],
+        ),
     ],
     ids=[
         "basic",
@@ -491,6 +504,7 @@ def _():
         "does_not_add_more_than_256_char",
         "char_replacement",
         "sampling_priority_0",
+        "value_tilda_and_equals_sign_replacement",
     ],
 )
 # since we are looping through a dict, we can't predict the order of some of the tags