NH-26751 NH-25139 Adjust propagator and sampler responsibilities #175

tammy-baylis-swi · 2023-07-13T20:31:14Z

XTraceOptions is now created by propagator.extract, or at sampler.should_sample but only as a fallback if none is in the current Otel context. All xto info is in the XTraceOptions struct under INTL_SWO_X_OPTIONS_KEY key in Otel context, so we no longer store under a separate INTL_SWO_SIGNATURE_KEY.

The XTraceOptions struct has been simplified so that it takes 2 optional headers (str) from incoming request's x-trace-options and x-trace-options-signature. It also only stores the original x-trace-options header if signature is provided. This was Raph's nice suggestion to save on space because options_header only comes in play for a signed TT request.

The sampler's helper calculate_trace_state is much simpler now! All is does is add xto info for piggyback into any xto response if needed. sw KV is no longer added to a (future) span's tracestate by the sampler.should_sample, while sw KV continues to be extracted and injected by propagator. This means:

sw can be in the tracestate of a completed span if it was extracted (propagator.extract) from a request with incoming, valid span context (scenario 4, 8) before should_sample is called.
sw will not be in tracestate of a completed span if there is no valid span context (scenario 1) -- so no more zeroed out span_id being used when not necessary.

Most of the changes here are unit/integration test updates which now all pass. I also did some manual/e2e testing with headers and traces described here (though not every one of our w3c context acceptance criteria): https://swicloud.atlassian.net/wiki/spaces/NIT/pages/3687877326/2023-07-13+Refactor+x-trace-options+usage

Please let me know if any questions, or if more e2e tests an/or a demo/walkthrough would help!

tammy-baylis-swi · 2023-07-18T01:01:10Z

solarwinds_apm/apm_constants.py

@@ -17,7 +17,6 @@
 INTL_SWO_TRACESTATE_KEY = "sw"
 INTL_SWO_X_OPTIONS_KEY = "sw_xtraceoptions"
 INTL_SWO_X_OPTIONS_RESPONSE_KEY = "xtrace_options_response"
-INTL_SWO_SIGNATURE_KEY = "sw_signature"


All xto info is in the XTraceOptions struct under INTL_SWO_X_OPTIONS_KEY key in Otel context, so we no longer store under a separate INTL_SWO_SIGNATURE_KEY.

tammy-baylis-swi · 2023-07-18T01:01:49Z

solarwinds_apm/traceoptions.py

+            # store original header for c-lib sample decision later
+            # only if signature_header exists
+            self.options_header = xtraceoptions_header


This was Raph's nice suggestion to save on space because options_header only comes in play for a signed TT request.

tammy-baylis-swi · 2023-07-18T01:02:47Z

solarwinds_apm/sampler.py

@@ -316,79 +317,31 @@ def create_xtraceoptions_response_value(

        return ";".join(response)

-    def create_new_trace_state(


calculate_trace_state is so much simpler that I figured we could compress and get rid of its helper create_new_trace_state

tammy-baylis-swi · 2023-07-18T01:04:50Z

solarwinds_apm/sampler.py

-        trace_state = TraceState(
-            [
-                (
-                    INTL_SWO_TRACESTATE_KEY,


The sampler no longer sets INTL_SWO_TRACESTATE_KEY (sw) in tracestate internally. We rely on the propagators to inject it into outgoing requests. In the case of valid remote parents, it's the propagators' extract that bring it to should_sample.

This makes me so happy

swi-jared · 2023-07-18T15:03:06Z

solarwinds_apm/propagator.py

-                xtraceoptions_header[0],
-            )
-
+        ) or [""]


Kind of a shame that the Getter doesn't provide a fallback

Indeed!!! 🙃

swi-jared · 2023-07-18T15:04:37Z

solarwinds_apm/sampler.py

+        """Calculates trace_state based on x-trace-options if provided -- for non-existent or remote parent spans only."""
+        # No valid parent i.e. root span, or parent is remote
+        if (
+            not parent_span_context.is_valid


Elsewhere I've seen you check whether parent_span_context is None. Is that not needed here?

Ah!!! Added in 0f3c67f

swi-jared · 2023-07-18T15:06:37Z

tests/integration/test_signed_tt.py

@@ -906,7 +910,7 @@ def test_signed_missing_xtraceoptions_header(self):

        # Verify x-trace-options-response response header not present
        # because no x-trace-options-header
-        assert "x-trace-options-response" not in resp.headers
+        # assert "x-trace-options-response" not in resp.headers  # !!!


No longer needed?

Actually it's needed, and it's failing! Gonna dig

Ok. Re-added the test in c5e0858 and it now passes. I think this is a requirement based on this page:

If an X-Trace-Options header was given, the agent will set a response header X-Trace-Options-Response to communicate status including the reason a trigger trace request was not traced and any ignored options (see request/response table below).

In the same commit I've added include_response to the XTraceOptions struct to make the check before calculating response header X-Trace-Options-Response, plus more unit tests.

raphael-theriault-swi · 2023-07-18T19:12:54Z

Looking at this too

raphael-theriault-swi

Looks good to me, happy to see this ! I'll led Jared finish his review

raphael-theriault-swi · 2023-07-18T21:28:17Z

solarwinds_apm/sampler.py

-        trace_state = TraceState(
-            [
-                (
-                    INTL_SWO_TRACESTATE_KEY,


This makes me so happy

swi-jared

LGTM, with a minor nit

swi-jared · 2023-07-18T22:17:41Z

solarwinds_apm/sampler.py

@@ -541,7 +495,13 @@ def should_sample(
        parent_span_context = get_current_span(
            parent_context
        ).get_span_context()
-        xtraceoptions = XTraceOptions(parent_context)
+
+        if not parent_context or not parent_context.get(


An optimization for this block might be:

xtraceoptions = XTraceOptions() if parent_context and parent_context.is_valid(): xtraceoptions = parent_context.get(INTL_SWO_OPTIONS_KEY) or xtraceoptions

This would remove the first parent_context.get call, which might not be hugely expensive on its own, but in aggregate the sampler might hit it quite a lot.

For Otel Python there isn't a Context.is_valid() function so we can't do parent_context.is_valid(). There is a SpanContext.is_valid() so we could do parent_span_context.is_valid() but I don't think this check does the same thing and the adjusted condition makes several of the integration/ tests fail.

When the Otel SDK calls should_sample, the Otel Context (parent_context) could be empty (falsy) or could contain KVs, e.g. what's set by extraction like a XTraceOptions struct, or what's at the _SPAN_KEY (used to get_current_span and get_span_context at L495-497)

If Otel SpanContext (parent_span_context) is valid or not valid, there could still be KVs in the Otel Context.

tammy-baylis-swi added 9 commits July 12, 2023 17:13

XTraceOptions init at propagator.extract not sampler

4e9efbf

Fix xtraceoptions if no parent_context

259bfff

Simplify sampler calculate_trace_state

d63b43b

Store xtraceoptions.option_header if signed tt

d946c60

lint

57cc409

extract default for xtraceoptions

03a9211

Adjust xto signature default and set

61d997c

Fix xto unit tests

43dfc65

Rm sw from tracestate in xto validation tests

dfc5959

tammy-baylis-swi force-pushed the NH-26751-refactor branch from 24a1e70 to dfc5959 Compare July 17, 2023 21:28

Fix test scenario 1,4,8

82c2998

tammy-baylis-swi force-pushed the NH-26751-refactor branch from bd62fe0 to 82c2998 Compare July 17, 2023 22:24

tammy-baylis-swi added 5 commits July 17, 2023 15:33

Re-add scen1 tracestate checks for no sw

50131b3

More scen8 assertion updates

7118964

Add comments

943db93

Fix signed tt tests

eca9631

Fix unsigned tt tests

806d4a5

tammy-baylis-swi changed the title ~~NH-26751 Refactor propagator and sampler~~ NH-26751 Adjust propagator and sampler responsibilities Jul 17, 2023

tammy-baylis-swi added 3 commits July 17, 2023 16:19

Fix propagator tests

71dddd0

Fix sampler tests

4136b71

Refactor sampler test

56449da

tammy-baylis-swi commented Jul 18, 2023

View reviewed changes

tammy-baylis-swi changed the title ~~NH-26751 Adjust propagator and sampler responsibilities~~ NH-26751 NH-25139 Adjust propagator and sampler responsibilities Jul 18, 2023

tammy-baylis-swi marked this pull request as ready for review July 18, 2023 01:10

tammy-baylis-swi requested a review from a team July 18, 2023 01:10

swi-jared reviewed Jul 18, 2023

View reviewed changes

raphael-theriault-swi reviewed Jul 18, 2023

View reviewed changes

tammy-baylis-swi added 2 commits July 18, 2023 14:42

Fix x-trace-options-response when only signature

c5e0858

Add parent_span_context none check

0f3c67f

tammy-baylis-swi requested a review from swi-jared July 18, 2023 22:02

swi-jared approved these changes Jul 18, 2023

View reviewed changes

tammy-baylis-swi merged commit 19d479f into main Jul 19, 2023

tammy-baylis-swi deleted the NH-26751-refactor branch July 19, 2023 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NH-26751 NH-25139 Adjust propagator and sampler responsibilities #175

NH-26751 NH-25139 Adjust propagator and sampler responsibilities #175

tammy-baylis-swi commented Jul 13, 2023 •

edited

Loading

tammy-baylis-swi Jul 18, 2023

tammy-baylis-swi Jul 18, 2023

tammy-baylis-swi Jul 18, 2023

tammy-baylis-swi Jul 18, 2023

raphael-theriault-swi Jul 18, 2023

swi-jared Jul 18, 2023

tammy-baylis-swi Jul 18, 2023

swi-jared Jul 18, 2023

tammy-baylis-swi Jul 18, 2023

swi-jared Jul 18, 2023

tammy-baylis-swi Jul 18, 2023

tammy-baylis-swi Jul 18, 2023

raphael-theriault-swi commented Jul 18, 2023

raphael-theriault-swi left a comment

raphael-theriault-swi Jul 18, 2023

swi-jared left a comment

swi-jared Jul 18, 2023

tammy-baylis-swi Jul 18, 2023

		@@ -316,79 +317,31 @@ def create_xtraceoptions_response_value(

		return ";".join(response)

		def create_new_trace_state(

NH-26751 NH-25139 Adjust propagator and sampler responsibilities #175

NH-26751 NH-25139 Adjust propagator and sampler responsibilities #175

Conversation

tammy-baylis-swi commented Jul 13, 2023 • 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

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

raphael-theriault-swi commented Jul 18, 2023

raphael-theriault-swi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swi-jared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tammy-baylis-swi commented Jul 13, 2023 •

edited

Loading