Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 20 commits into from
Jul 19, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4e9efbf
XTraceOptions init at propagator.extract not sampler
tammy-baylis-swi Jul 12, 2023
259bfff
Fix xtraceoptions if no parent_context
tammy-baylis-swi Jul 13, 2023
d63b43b
Simplify sampler calculate_trace_state
tammy-baylis-swi Jul 15, 2023
d946c60
Store xtraceoptions.option_header if signed tt
tammy-baylis-swi Jul 15, 2023
57cc409
lint
tammy-baylis-swi Jul 15, 2023
03a9211
extract default for xtraceoptions
tammy-baylis-swi Jul 15, 2023
61d997c
Adjust xto signature default and set
tammy-baylis-swi Jul 17, 2023
43dfc65
Fix xto unit tests
tammy-baylis-swi Jul 17, 2023
dfc5959
Rm sw from tracestate in xto validation tests
tammy-baylis-swi Jul 17, 2023
82c2998
Fix test scenario 1,4,8
tammy-baylis-swi Jul 17, 2023
50131b3
Re-add scen1 tracestate checks for no sw
tammy-baylis-swi Jul 17, 2023
7118964
More scen8 assertion updates
tammy-baylis-swi Jul 17, 2023
943db93
Add comments
tammy-baylis-swi Jul 17, 2023
eca9631
Fix signed tt tests
tammy-baylis-swi Jul 17, 2023
806d4a5
Fix unsigned tt tests
tammy-baylis-swi Jul 17, 2023
71dddd0
Fix propagator tests
tammy-baylis-swi Jul 17, 2023
4136b71
Fix sampler tests
tammy-baylis-swi Jul 18, 2023
56449da
Refactor sampler test
tammy-baylis-swi Jul 18, 2023
c5e0858
Fix x-trace-options-response when only signature
tammy-baylis-swi Jul 18, 2023
0f3c67f
Add parent_span_context none check
tammy-baylis-swi Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion solarwinds_apm/apm_constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

INTL_SWO_DEFAULT_TRACES_EXPORTER = "solarwinds_exporter"
INTL_SWO_TRACECONTEXT_PROPAGATOR = "tracecontext"
INTL_SWO_PROPAGATOR = "solarwinds_propagator"
Expand Down
27 changes: 8 additions & 19 deletions solarwinds_apm/propagator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@
from opentelemetry.trace.span import TraceState

from solarwinds_apm.apm_constants import (
INTL_SWO_SIGNATURE_KEY,
INTL_SWO_TRACESTATE_KEY,
INTL_SWO_X_OPTIONS_KEY,
)
from solarwinds_apm.traceoptions import XTraceOptions
from solarwinds_apm.w3c_transformer import W3CTransformer

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -46,27 +46,16 @@ def extract(

xtraceoptions_header = getter.get(
carrier, self._XTRACEOPTIONS_HEADER_NAME
)
if xtraceoptions_header:
context.update({INTL_SWO_X_OPTIONS_KEY: xtraceoptions_header[0]})
logger.debug(
"Extracted %s as %s: %s",
self._XTRACEOPTIONS_HEADER_NAME,
INTL_SWO_X_OPTIONS_KEY,
xtraceoptions_header[0],
)

) or [""]
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed!!! 🙃

signature_header = getter.get(
carrier, self._XTRACEOPTIONS_SIGNATURE_HEADER_NAME
) or [""]
xtraceoptions = XTraceOptions(
xtraceoptions_header[0],
signature_header[0],
)
if signature_header:
context.update({INTL_SWO_SIGNATURE_KEY: signature_header[0]})
logger.debug(
"Extracted %s as %s: %s",
self._XTRACEOPTIONS_SIGNATURE_HEADER_NAME,
INTL_SWO_SIGNATURE_KEY,
signature_header[0],
)

context.update({INTL_SWO_X_OPTIONS_KEY: xtraceoptions})
return context

def inject(
Expand Down
86 changes: 23 additions & 63 deletions solarwinds_apm/sampler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
INTL_SWO_COMMA_W3C_SANITIZED,
INTL_SWO_EQUALS_W3C_SANITIZED,
INTL_SWO_TRACESTATE_KEY,
INTL_SWO_X_OPTIONS_KEY,
INTL_SWO_X_OPTIONS_RESPONSE_KEY,
)
from solarwinds_apm.traceoptions import XTraceOptions
Expand Down Expand Up @@ -316,79 +317,32 @@ def create_xtraceoptions_response_value(

return ";".join(response)

def create_new_trace_state(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

def calculate_trace_state(
self,
decision: dict,
parent_span_context: SpanContext,
xtraceoptions: Optional[XTraceOptions] = None,
) -> TraceState:
"""Creates new TraceState based on trace decision, parent span id,
and x-trace-options if provided"""
trace_state = TraceState(
[
(
INTL_SWO_TRACESTATE_KEY,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

This makes me so happy

W3CTransformer.sw_from_span_and_decision(
parent_span_context.span_id,
W3CTransformer.trace_flags_from_int(
decision["do_sample"]
),
),
)
]
)
if xtraceoptions and xtraceoptions.options_header:
"""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
or not parent_span_context.is_valid
or not parent_span_context.trace_state
):
trace_state = TraceState()
else:
trace_state = parent_span_context.trace_state
# Update with x-trace-options-response if applicable
# Not a propagated header, so always add at should_sample
if xtraceoptions and xtraceoptions.include_response:
trace_state = trace_state.add(
INTL_SWO_X_OPTIONS_RESPONSE_KEY,
self.create_xtraceoptions_response_value(
decision, parent_span_context, xtraceoptions
),
)
logger.debug("Created new trace_state: %s", trace_state)
return trace_state

def calculate_trace_state(
self,
decision: dict,
parent_span_context: SpanContext,
xtraceoptions: Optional[XTraceOptions] = None,
) -> TraceState:
"""Calculates trace_state based on parent span context, trace decision,
and 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:
trace_state = self.create_new_trace_state(
decision, parent_span_context, xtraceoptions
)
else:
trace_state = parent_span_context.trace_state
if not trace_state:
# tracestate nonexistent/non-parsable
trace_state = self.create_new_trace_state(
decision, parent_span_context, xtraceoptions
)
else:
# Update trace_state with span_id and sw trace_flags
trace_state = trace_state.update(
INTL_SWO_TRACESTATE_KEY,
W3CTransformer.sw_from_span_and_decision(
parent_span_context.span_id, # NOTE: This is a placeholder before propagator inject
W3CTransformer.trace_flags_from_int(
decision["do_sample"]
),
),
)
# Update trace_state with x-trace-options-response
# Not a propagated header, so always an add
if xtraceoptions and xtraceoptions.options_header:
trace_state = trace_state.add(
INTL_SWO_X_OPTIONS_RESPONSE_KEY,
self.create_xtraceoptions_response_value(
decision, parent_span_context, xtraceoptions
),
)
logger.debug("Updated trace_state: %s", trace_state)
logger.debug("Calculated trace_state: %s", trace_state)
return trace_state

def add_tracestate_capture_to_attributes_dict(
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

INTL_SWO_X_OPTIONS_KEY
):
xtraceoptions = XTraceOptions()
else:
xtraceoptions = parent_context.get(INTL_SWO_X_OPTIONS_KEY)

liboboe_decision = self.calculate_liboboe_decision(
parent_span_context, name, kind, attributes, xtraceoptions
Expand Down
38 changes: 16 additions & 22 deletions solarwinds_apm/traceoptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,6 @@

import logging
import re
import typing

from opentelemetry.context.context import Context

from solarwinds_apm.apm_constants import (
INTL_SWO_SIGNATURE_KEY,
INTL_SWO_X_OPTIONS_KEY,
)

logger = logging.getLogger(__name__)

Expand All @@ -31,28 +23,34 @@ class XTraceOptions:
# pylint: disable=too-many-branches,too-many-statements
def __init__(
self,
context: typing.Optional[Context] = None,
xtraceoptions_header: str = "",
signature_header: str = "",
):
"""
Args:
context: OTel context that may contain OTEL_CONTEXT_SW_OPTIONS_KEY,OTEL_CONTEXT_SW_SIGNATURE_KEY
xtraceoptions_header: extracted request header value
signature_header: extracted request header value
"""
self.ignored = []
self.options_header = ""
self.signature = None
self.signature = ""
self.custom_kvs = {}
self.sw_keys = ""
self.trigger_trace = 0
self.timestamp = 0
self.include_response = False

if not context:
return
options_header = context.get(INTL_SWO_X_OPTIONS_KEY, "")
# store original header for sample decision later
self.options_header = options_header
if signature_header:
self.signature = signature_header
# store original header for c-lib sample decision later
# only if signature_header exists
self.options_header = xtraceoptions_header
Comment on lines +45 to +47
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


if options_header:
traceoptions = re.split(r";+", options_header)
if xtraceoptions_header:
# If x-trace-options header given, set response header
self.include_response = True

traceoptions = re.split(r";+", xtraceoptions_header)
for option in traceoptions:
# KVs (e.g. sw-keys or custom-key1) are assigned by equals
option_kv = option.split("=", 1)
Expand Down Expand Up @@ -117,7 +115,3 @@ def __init__(
"Some x-trace-options were ignored: %s",
", ".join(self.ignored),
)

options_signature = context.get(INTL_SWO_SIGNATURE_KEY, None)
if options_signature:
self.signature = options_signature
16 changes: 8 additions & 8 deletions tests/integration/test_scenario_1.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,10 @@ def test_scenario_1_sampled(self):
assert span_client.name == "HTTP GET"
assert span_client.kind == trace_api.SpanKind.CLIENT

# Check root span tracestate has `sw` key
# In this test we know its value will have invalid span_id
expected_trace_state = trace_api.TraceState([("sw", "0000000000000000-01")])
assert span_server.context.trace_state == expected_trace_state
# Check root span tracestate has no `sw` key
# because no valid parent context
expected_trace_state = trace_api.TraceState([])
assert span_server.context.trace_state.get("sw") == expected_trace_state.get("sw") # None

# Check root span attributes
# :present:
Expand All @@ -116,10 +116,10 @@ def test_scenario_1_sampled(self):
assert not "sw.tracestate_parent_id" in span_server.attributes
assert not "SWKeys" in span_server.attributes

# Check outgoing request tracestate has `sw` key
# In this test we know its value will also have invalid span_id
expected_trace_state = trace_api.TraceState([("sw", "0000000000000000-01")])
assert span_client.context.trace_state == expected_trace_state
# Check outgoing request span tracestate has no `sw` key
# because no valid parent context
expected_trace_state = trace_api.TraceState([])
assert span_client.context.trace_state.get("sw") == expected_trace_state.get("sw") # None

# Check outgoing request span attributes
# :absent:
Expand Down
12 changes: 6 additions & 6 deletions tests/integration/test_scenario_4.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,11 @@ def test_scenario_4_sampled(self):
assert "{:032x}".format(span_client.context.trace_id) == trace_id

# Check service entry span tracestate has `sw` key
# In this test it should be span_id, traceflags from extracted traceparent
# In this test it should be tracestate_span_id, traceflags from extracted traceparent
expected_trace_state = trace_api.TraceState([
("sw", "{}-{}".format(span_id, trace_flags))
("sw", "{}-{}".format(tracestate_span, trace_flags))
])
assert span_server.context.trace_state == expected_trace_state
assert span_server.context.trace_state.get("sw") == expected_trace_state.get("sw")

# Check service entry span attributes
# :present:
Expand All @@ -150,11 +150,11 @@ def test_scenario_4_sampled(self):
assert not "SWKeys" in span_server.attributes

# Check outgoing request tracestate has `sw` key
# In this test it should also be span_id, traceflags from extracted traceparent
# In this test it should also be tracestate_span_id, traceflags from extracted traceparent
expected_trace_state = trace_api.TraceState([
("sw", "{}-{}".format(span_id, trace_flags))
("sw", "{}-{}".format(tracestate_span, trace_flags))
])
assert span_client.context.trace_state == expected_trace_state
assert span_client.context.trace_state.get("sw") == expected_trace_state.get("sw")

# Check outgoing request span attributes
# :absent:
Expand Down
Loading