-
Notifications
You must be signed in to change notification settings - Fork 1
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
NH-2313 Add basic TraceState handling and W3C trace context propagation #11
Changes from 47 commits
23e4f8e
b591d4f
9ceb4df
a5701a1
a6533bc
05b8887
a7c6244
d91e276
ae29003
394ae52
f6ff7f4
c380527
5659f5b
f17b2b8
52e655a
69a487a
cb5800e
537cf0a
88529a1
761e34f
07ec508
77dbfb6
c4ca055
30899bf
7e4e8c9
d9c0e63
5204536
12d4cbd
bd315a6
8f5a215
60522b7
3e175ba
38639c9
f95bf46
af11112
322949b
ff201c7
05f03c7
3b62bc5
1dfd489
99d2fc4
32df3d0
7444d43
84a863e
de88565
ec2c317
59b1bac
45f1910
552398c
58acd70
783cac0
b58e29b
b08517e
f7f487a
3aad13d
ffccf28
3a90bdb
118c1bc
23a12a2
526df98
7077e6a
f9171c8
0aef14c
c77afc9
68a0e63
7b75e65
600e5b2
ede5686
2842209
7ffb1a9
02bde11
54a0e7d
1241e7a
1b830a1
73be566
126e030
01f1e98
7e758df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
|
||
from opentelemetry_distro_solarwinds.extension.oboe import (Context, Metadata, | ||
Reporter) | ||
from opentelemetry_distro_solarwinds.ot_ao_transformer import transform_id | ||
from opentelemetry_distro_solarwinds.w3c_transformer import W3CTransformer | ||
|
||
logger = logging.getLogger(__file__) | ||
|
||
|
@@ -36,15 +36,15 @@ def export(self, spans): | |
md = self._build_metadata(span.get_span_context()) | ||
if span.parent and span.parent.is_valid: | ||
# If there is a parent, we need to add an edge to this parent to this entry event | ||
logger.debug("Continue trace from %s", md.toString()) | ||
logger.debug("Continue trace from {}".format(md.toString())) | ||
parent_md = self._build_metadata(span.parent) | ||
evt = Context.startTrace(md, int(span.start_time / 1000), | ||
parent_md) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit and inherited code, but let's rename this to something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's going to be a separate liboboe PR (oboe itself or the SWIG interface only if possible?), related to NH-9150, NH-7246, and this comment: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2823618801/liboboe+and+OTel+Python+custom-distro?focusedCommentId=2826010703#comment-2826010703 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've now addressed this in 0aef14c by switching to use the new |
||
else: | ||
# In OpenTelemrtry, there are no events with individual IDs, but only a span ID | ||
# and trace ID. Thus, the entry event needs to be generated such that it has the | ||
# same op ID as the span ID of the OTel span. | ||
logger.debug("Start a new trace %s", md.toString()) | ||
logger.debug("Start a new trace {}".format(md.toString())) | ||
evt = Context.startTrace(md, int(span.start_time / 1000)) | ||
evt.addInfo('Layer', span.name) | ||
evt.addInfo('Language', 'Python') | ||
|
@@ -122,4 +122,6 @@ def _initialize_solarwinds_reporter(self): | |
|
||
@staticmethod | ||
def _build_metadata(span_context): | ||
return Metadata.fromString(transform_id(span_context)) | ||
return Metadata.fromString( | ||
W3CTransformer.traceparent_from_context(span_context) | ||
) |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
import logging | ||
import typing | ||
|
||
from opentelemetry import trace | ||
from opentelemetry.context.context import Context | ||
from opentelemetry.propagators import textmap | ||
from opentelemetry.trace.span import TraceState | ||
|
||
from opentelemetry_distro_solarwinds.traceoptions import XTraceOptions | ||
from opentelemetry_distro_solarwinds.w3c_transformer import W3CTransformer | ||
|
||
logger = logging.getLogger(__file__) | ||
|
||
class SolarWindsPropagator(textmap.TextMapPropagator): | ||
"""Extracts and injects SolarWinds headers for trace propagation. | ||
Must be used in composite with TraceContextTextMapPropagator. | ||
""" | ||
_TRACESTATE_HEADER_NAME = "tracestate" | ||
_XTRACEOPTIONS_HEADER_NAME = "x-trace-options" | ||
_XTRACEOPTIONS_SIGNATURE_HEADER_NAME = "x-trace-options-signature" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I a little bit forget the header name: is it all lowercased There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As Alex mentioned in standup, field names are case-insensitive: https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2 I also did a quick test by making requests to the instrumented testbed Django A app with either |
||
|
||
def extract( | ||
self, | ||
carrier: textmap.CarrierT, | ||
context: typing.Optional[Context] = None, | ||
getter: textmap.Getter = textmap.default_getter, | ||
) -> Context: | ||
"""Extracts sw trace options and signature from carrier into OTel | ||
Context. Note: tracestate is extracted by TraceContextTextMapPropagator | ||
""" | ||
if context is None: | ||
context = Context() | ||
|
||
xtraceoptions_header = getter.get( | ||
carrier, | ||
self._XTRACEOPTIONS_HEADER_NAME | ||
) | ||
if not xtraceoptions_header: | ||
logger.debug("No xtraceoptions to extract; ignoring signature") | ||
return context | ||
logger.debug("Extracted xtraceoptions_header: {}".format( | ||
xtraceoptions_header[0] | ||
)) | ||
|
||
signature_header = getter.get( | ||
carrier, | ||
self._XTRACEOPTIONS_SIGNATURE_HEADER_NAME | ||
) | ||
if signature_header: | ||
xtraceoptions = XTraceOptions( | ||
context, | ||
xtraceoptions_header[0], | ||
signature_header[0], | ||
) | ||
else: | ||
xtraceoptions = XTraceOptions( | ||
context, | ||
xtraceoptions_header[0], | ||
) | ||
context.update(dict(xtraceoptions)) | ||
return context | ||
|
||
def inject( | ||
self, | ||
carrier: textmap.CarrierT, | ||
context: typing.Optional[Context] = None, | ||
setter: textmap.Setter = textmap.default_setter, | ||
) -> None: | ||
"""Injects sw tracestate and trace options from SpanContext into carrier for HTTP request""" | ||
span = trace.get_current_span(context) | ||
span_context = span.get_span_context() | ||
trace_state = span_context.trace_state | ||
sw_value = W3CTransformer.sw_from_context(span_context) | ||
|
||
# Prepare carrier with context's or new tracestate | ||
if trace_state: | ||
# Check if trace_state already contains sw KV | ||
if "sw" in trace_state.keys(): | ||
# If so, modify current span_id and trace_flags, and move to beginning of list | ||
logger.debug("Updating trace state for injection with {}".format(sw_value)) | ||
trace_state = trace_state.update("sw", sw_value) | ||
|
||
else: | ||
# If not, add sw KV to beginning of list | ||
logger.debug("Adding KV to trace state for injection with {}".format(sw_value)) | ||
trace_state = trace_state.add("sw", sw_value) | ||
else: | ||
logger.debug("Creating new trace state for injection with {}".format(sw_value)) | ||
trace_state = TraceState([("sw", sw_value)]) | ||
|
||
setter.set( | ||
carrier, self._TRACESTATE_HEADER_NAME, trace_state.to_header() | ||
) | ||
|
||
@property | ||
def fields(self) -> typing.Set[str]: | ||
"""Returns a set with the fields set in `inject`""" | ||
return { | ||
self._TRACESTATE_HEADER_NAME | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import logging | ||
import typing | ||
|
||
from opentelemetry import trace | ||
from opentelemetry.context.context import Context | ||
from opentelemetry.instrumentation.propagators import ResponsePropagator | ||
from opentelemetry.propagators import textmap | ||
from opentelemetry.trace.span import TraceState | ||
|
||
from opentelemetry_distro_solarwinds.traceoptions import XTraceOptions | ||
from opentelemetry_distro_solarwinds.w3c_transformer import W3CTransformer | ||
|
||
logger = logging.getLogger(__file__) | ||
|
||
class SolarWindsTraceResponsePropagator(ResponsePropagator): | ||
"""Propagator that injects SW values into HTTP responses""" | ||
_HTTP_HEADER_ACCESS_CONTROL_EXPOSE_HEADERS = "Access-Control-Expose-Headers" | ||
_XTRACE_HEADER_NAME = "x-trace" | ||
_XTRACEOPTIONS_RESPONSE_HEADER_NAME = "x-trace-options-response" | ||
|
||
def inject( | ||
self, | ||
carrier: textmap.CarrierT, | ||
context: typing.Optional[Context] = None, | ||
setter: textmap.Setter = textmap.default_setter, | ||
) -> None: | ||
"""Injects x-trace and options-response into the HTTP response carrier.""" | ||
span = trace.get_current_span(context) | ||
span_context = span.get_span_context() | ||
if span_context == trace.INVALID_SPAN_CONTEXT: | ||
return | ||
|
||
x_trace = W3CTransformer.traceparent_from_context(span_context) | ||
setter.set( | ||
carrier, | ||
self._XTRACE_HEADER_NAME, | ||
x_trace, | ||
) | ||
exposed_headers = [self._XTRACE_HEADER_NAME] | ||
|
||
xtraceoptions_response = self.recover_response_from_tracestate( | ||
span_context.trace_state | ||
) | ||
if xtraceoptions_response: | ||
exposed_headers.append("{}".format( | ||
self._XTRACEOPTIONS_RESPONSE_HEADER_NAME | ||
)) | ||
setter.set( | ||
carrier, | ||
self._XTRACEOPTIONS_RESPONSE_HEADER_NAME, | ||
xtraceoptions_response, | ||
) | ||
setter.set( | ||
carrier, | ||
self._HTTP_HEADER_ACCESS_CONTROL_EXPOSE_HEADERS, | ||
",".join(exposed_headers), | ||
) | ||
|
||
def recover_response_from_tracestate( | ||
self, | ||
tracestate: TraceState | ||
) -> str: | ||
"""Use tracestate to recover xtraceoptions response by | ||
converting delimiters: | ||
`####` becomes `=` | ||
`....` becomes `,` | ||
""" | ||
sanitized = tracestate.get( | ||
XTraceOptions.get_sw_xtraceoptions_response_key(), | ||
None | ||
) | ||
if not sanitized: | ||
return | ||
return sanitized.replace("####", "=").replace("....", ",") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also set our custom request propagator here, rather than needing the customor to configure it in? https://github.com/open-telemetry/opentelemetry-python-contrib/blob/22b069baaec15b21bc1879874d1a244210bc6e1b/opentelemetry-distro/src/opentelemetry/distro/__init__.py#L34 shows using an environment variable to configure, there's probably also a programmatic way similar to how our custom sampler and exporter is configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 783cac0 I've added this to the distro
_configure
method to always use a Composite oftracecontext,baggage,solarwinds
propagators, instead of needing the customer to setOTEL_PROPAGATORS
env var:This does prevent the customer from customizing the CompositePropagator further... Actually I don't know how a customer would do this unless they contribute to our custom-distro. Is the above the way we want to go instead of
os.environ.setdefault
with added checks?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point re: configurability! It would be more flexible if we allow the customer to configure components such as propagator and exporter -- might introduce more support issues if there's negative interference but one major use case is someone comparing (or even transitioning to) our product could for example use our agent but also keep exporting to another vendor platform.
Digging around, there are quite a few env vars defined by OTel:
And quick scan of these threads seems custom distro configuring via env var is the approach talked about by the community: open-telemetry/opentelemetry-python-contrib#551 and open-telemetry/opentelemetry-python#1937 (comment)
So I think it makes sense to allow the propagator and exporter to be customized by the customer, on top of our distro. No need to support that for the sampler or tracer provider for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Researching and working on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @cheempz I've set up one way to do this in f9171c8 and 7077e6a. What do you think?
I've set up custom-distro to always use
tracecontext,baggage,solarwinds_propagator
first and in that order. Anything else inOTEL_PROPAGATORS
is used to set up theCompositePropagator
after them. If any of the propagators can't be loaded, exception is raised. This is the same behaviour as opentelemetry-api: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/propagate/__init__.py#L132-L144 This means the propagators have to be implemented/properly-mocked ones and not just "foo/bar".I think OTel Python supports having only one exporter (no composites), so our custom-distro uses
OTEL_TRACES_EXPORTER
else default to ours with entrypoint namesolarwinds_exporter
(set insetup.cfg
) If whichever exporter is specified can't be loaded, exception is raised like above.To test this I've updated the testbed in this PR: https://github.com/appoptics/opentelemetry-python-testbed/pull/21 One thing that makes me happy is that we can get customers to install
opentelemetry-exporter-otlp-proto-grpc
on their end if they want to switch Exporter tootlp_proto_grpc
, which is one of the common OTel Python ones: https://opentelemetry.io/docs/instrumentation/python/exporters/In 7077e6a and the testbed PR I've also upped OTel Python version to
1.11.0/0.30b0
which was released yesterday. In standup I mentioned I thought the update might break custom-distro setup, but not the case! I still updated anyway since we want to keep up with the latest version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add the above info to the ongoing NH Python info page: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/2867726621/NH+Python+Troubleshooting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UPDATE: This behaviour will change in TWO PRS:
#14
#15
Please see those PRs and their linked test docs. The NH Python info page has also been updated to reflect.