feat: trace improvements #450

daniel-sanche · 2021-11-23T23:15:12Z

This PR improves trace detection by adding the following features:

adds support for the open W3 traceparent header format
adds support for the trace_sampled field

These changes should bring the python library in line with how nodejs currently works.

I also added a number of unit, system, and environment tests to make sure the new features work as expected throughout the stack

Fixes #321

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…ging into trace_improvements

minherz

lgtm: high art of regex, bravo!

minherz · 2021-12-07T10:45:08Z

google/cloud/logging_v2/handlers/_helpers.py

-_DJANGO_TRACE_HEADER = "HTTP_X_CLOUD_TRACE_CONTEXT"
+_DJANGO_XCLOUD_TRACE_HEADER = "HTTP_X_CLOUD_TRACE_CONTEXT"
+_DJANGO_TRACEPARENT = "HTTP_TRACEPARENT"
 _DJANGO_USERAGENT_HEADER = "HTTP_USER_AGENT"
 _DJANGO_REMOTE_ADDR_HEADER = "REMOTE_ADDR"
 _DJANGO_REFERER_HEADER = "HTTP_REFERER"
-_FLASK_TRACE_HEADER = "X_CLOUD_TRACE_CONTEXT"
+_FLASK_XCLOUD_TRACE_HEADER = "X_CLOUD_TRACE_CONTEXT"
+_FLASK_TRACEPARENT = "TRACEPARENT"


nit: since these headers are standardized, is there a need to maintain an instance per web server framework?

Yeah, for some reason django uses "HTTP_" as a prefix when reading most HTTP headers.

At some point as we add more framework support, we may want to revisit this to add the prefix later in django code instead of in the constant, but for now I'd prefer to leave it as-is

minherz · 2021-12-07T10:46:08Z

google/cloud/logging_v2/handlers/_helpers.py

-            Data related to the current http request, trace_id, and span_id for
-            the request. All fields will be None if a django request isn't
-            found.
+        Tuple[Optional[dict], Optional[str], Optional[str], bool]:


nit: shouldn't bool be optional too?

No, the LogEntry spec says if the sampling decision is uncertain, you should default the trace_sampled field to False. So this function will always return True or False for that field

FWIW, the code immediately below returns None.

Oh good catch! I started with it being optional before switching. I'll look through the code again

minherz · 2021-12-07T10:59:29Z

google/cloud/logging_v2/handlers/_helpers.py

-            Data related to the current http request, trace_id, and span_id for
-            the request. All fields will be None if a django request isn't
-            found.
+        Tuple[Optional[dict], Optional[str], Optional[str], bool]:


nit: same question about Optional[] here

minherz · 2021-12-07T11:00:47Z

google/cloud/logging_v2/handlers/_helpers.py

+    # see https://www.w3.org/TR/trace-context/ for W3C traceparent format
+    if header:
+        try:
+            VERSION_PART = r"(?!ff)[a-f\d]{2}"


nit: right now the version must be 00. other versions are considered invalid.

How are you handling other versions? Just ignoring anything other than 00?

It looks like nodejs is assuming forward-compatibility, based on this part of the spec ("The current version of this specification assumes that future versions of the traceparent header will be additive to the current one."). I'm fine with locking to version 00 for now though if we decide that's safer. Let me know if you have thoughts.

minherz · 2021-12-07T11:06:52Z

google/cloud/logging_v2/handlers/_helpers.py

-            header_suffix = split_header[1]
-            # the span is the set of alphanumeric characters after the /
-            span_id = re.findall(r"^\w+", header_suffix)[0]
+            regex = r"([\w-]+)?(\/?([\w-]+))?(;?o=(\d))?"


nit: according to our doc TRACE_ID is 32-character hexadec value that can be presented via acceptible UUID formats. SPAN_ID is a decimal value. adding it because tracing of the w3c was more strict. 🙂

Yeah, I'm kind of curious on your thoughts here. I made the w3c code more strict, because the spec is pretty explicit about dropping invalid headers (which makes sense for an open spec if you want to ensure all implementations are interoperable).

I thought we might want to be a bit more permissible in the X-Cloud-Trace-Context parsing, and leave the correct formatting to the user. But maybe it would make sense to be strict for both?

minherz · 2021-12-07T11:08:37Z

tests/system/test_system.py

@@ -534,6 +534,7 @@ def test_handlers_w_extras(self):
        extra = {
            "trace": "123",
            "span_id": "456",
+            "trace_sampled": True,


nit: do you want to add a regression test (sampled=false or omitted)?

good point, I added a couple more assertions to this file

minherz · 2021-12-07T11:12:37Z

tests/unit/handlers/test_handlers.py

+        expected_agent = "Mozilla/5.0"
+        expected_trace = "4bf92f3577b34da6a3ce929d0e0e4736"
+        expected_span = "00f067aa0ba902b7"
+        combined_trace = f"00-{expected_trace}-{expected_span}-03"


nit: you might want to set the flags field to 01 to keep aligned with other flags of the standard.
on the same account would you like to add sampling=false test?

trace_sampled should be True if the last bit in the flags byte string is True. I wanted to test the more complex case of "0...011" instead of the simple case of 0...001. (The 01 case and others are thoroughly tested elsewhere in test__helpers. test_handlers is slightly more high level)

minherz · 2021-12-07T11:15:20Z

tests/unit/handlers/test_structured_log.py

+        expected_agent = "Mozilla/5.0"
+        expected_trace = "4bf92f3577b34da6a3ce929d0e0e4736"
+        expected_span = "00f067aa0ba902b7"
+        trace_header = f"00-{expected_trace}-{expected_span}-09"


nit: does flags=09 has a special meaning?

Like above, I wanted to test with a flags field with multiple bits set (b1001), to make it more interesting. Most of the trace_sampled tests are in the test__helpers.py file.

google/cloud/logging_v2/handlers/_helpers.py

losalex

LGTM

daniel-sanche added 5 commits November 22, 2021 14:01

added trace_sampled detection

9fe2d8a

implemented traceparent detection

3f464b5

check for both header fields

c77657a

fixed GKE handler

310ae13

always return 4-tuple

98bd28c

daniel-sanche requested review from a team as code owners November 23, 2021 23:15

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 23, 2021

blunderbuss-gcf bot assigned daniel-sanche Nov 23, 2021

product-auto-label bot added the api: logging Issues related to the googleapis/python-logging API. label Nov 23, 2021

daniel-sanche changed the base branch from main to v3.0.0 November 23, 2021 23:15

daniel-sanche marked this pull request as draft November 23, 2021 23:15

gcf-owl-bot bot and others added 10 commits November 23, 2021 23:17

🦉 Updates from OwlBot

93d0e61

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

fixed unit tests

e8785cb

added tests for w3c traceparent

3fd0899

added tests for different trace types

899dd93

added trace_sampled to structured log handler

cb8bdfb

added tests for structured log handler integration

d652588

Merge branch 'trace_improvements' of github.com:googleapis/python-log…

0563239

…ging into trace_improvements

updated submodule

0d53025

lint fixes

c9bc772

added extra system test

d9b4f0a

daniel-sanche changed the title ~~[DRAFT] feat: trace improvements~~ feat: trace improvements Dec 2, 2021

daniel-sanche added 2 commits December 2, 2021 14:44

updated submodule

8662338

fixed structured log handler string interpretation issue

a874e9a

daniel-sanche marked this pull request as ready for review December 2, 2021 23:47

daniel-sanche mentioned this pull request Dec 3, 2021

fix: enforce w3c trace context value validation googleapis/java-logging#777

Merged

updated submodule

0286522

minherz approved these changes Dec 7, 2021

View reviewed changes

minherz mentioned this pull request Dec 7, 2021

feat: add json_fields extras argument for adding to jsonPayload #447

Merged

losalex reviewed Dec 7, 2021

View reviewed changes

google/cloud/logging_v2/handlers/_helpers.py Outdated Show resolved Hide resolved

google/cloud/logging_v2/handlers/_helpers.py Outdated Show resolved Hide resolved

daniel-sanche added 7 commits December 7, 2021 12:07

added more test cases

b30f9e7

updated submodule

7e521db

Merge remote-tracking branch 'origin/v3.0.0' into trace_improvements

e395a38

default trace_sampled to False

438b3ee

updated submodule

a6c6047

updated submodule

bcbb318

changed logic for trace extraction

b3acb10

losalex approved these changes Dec 7, 2021

View reviewed changes

daniel-sanche merged commit 08d35bb into v3.0.0 Dec 10, 2021

daniel-sanche deleted the trace_improvements branch December 10, 2021 22:59

daniel-sanche mentioned this pull request Dec 10, 2021

chore: merge v3.0.0 staging branch into main #455

Merged

daniel-sanche added a commit that referenced this pull request Jan 27, 2022

feat: trace improvements (#450)

a24a515

daniel-sanche added a commit that referenced this pull request Jan 27, 2022

feat: trace improvements (#450)

e0c5fc0

release-please bot mentioned this pull request Jan 27, 2022

chore(main): release 3.0.0 #473

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: trace improvements #450

feat: trace improvements #450

daniel-sanche commented Nov 23, 2021 •

edited

Loading

minherz left a comment

minherz Dec 7, 2021

daniel-sanche Dec 7, 2021

minherz Dec 7, 2021

daniel-sanche Dec 7, 2021

tseaver Dec 7, 2021

daniel-sanche Dec 7, 2021

minherz Dec 7, 2021

minherz Dec 7, 2021

daniel-sanche Dec 7, 2021

minherz Dec 7, 2021

daniel-sanche Dec 7, 2021 •

edited

Loading

minherz Dec 7, 2021

daniel-sanche Dec 7, 2021

minherz Dec 7, 2021

daniel-sanche Dec 7, 2021

minherz Dec 7, 2021

daniel-sanche Dec 7, 2021

losalex left a comment

feat: trace improvements #450

feat: trace improvements #450

Conversation

daniel-sanche commented Nov 23, 2021 • edited Loading

minherz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-sanche Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losalex left a comment

Choose a reason for hiding this comment

daniel-sanche commented Nov 23, 2021 •

edited

Loading

daniel-sanche Dec 7, 2021 •

edited

Loading