-
Notifications
You must be signed in to change notification settings - Fork 649
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
Add Datadog propagator #705
Add Datadog propagator #705
Conversation
f0b691e
to
7e9bb8c
Compare
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.
LGTM! From a code perspective.
I raised a couple side questions around DD and other propagators in practice.
Another question that comes to mind: for an ecosystem that uses purely OT, how will the DD_ORIGIN be set in the first place? Will an http client be responsible for adding the field, if it doesn't already exist?
I see that all the code here is strictly propagating the DD_ORIGIN value, rather than originating it.
ext/opentelemetry-ext-datadog/src/opentelemetry/ext/datadog/propagator.py
Outdated
Show resolved
Hide resolved
return str(span_id) | ||
|
||
|
||
def extract_first_element( |
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.
is this a utility method available in the API? I know it's good to not couple too heavily, but this feels like boilerplate for propagators.
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 see it in opentelemetry.correlationcontext.propagation
as well as the b3 code. Could be pulled out as a generally applicable http util method. Suggestions on where that might be? Could do this refactoring in a follow-up PR.
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.
correlationcontext is interesting. I don't have a strong opinion, maybe opentelemetry.propagation.utils?
# add origin to root span | ||
origin = _get_origin(span) | ||
if origin and parent_id == 0: | ||
datadog_span.set_tag(DD_ORIGIN, origin) |
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.
an aside, but how is the origin key used by DataDog? I wonder if this is something to raise up to the specification SIG, if there's value in another key that is not appropriately propagated.
This also raises an issue for those who want to use open propagation specifications and DataDog at the same time. If one does want to use DataDog in it's full capacity, I presume that will require the setting of the DD_ORIGIN value. For those want to switch to DataDog despite using an existing propagation spec, they may not get the full functionality unless they switch all services to use this propagator.
Thoughts?
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.
This is used to link traces to Synthetics tests: https://docs.datadoghq.com/synthetics/apm/#how-are-traces-linked-to-tests
As for using Datadog alongside an open propagation specification, I had been thinking that this be accomplished by adding the Datadog format to the composite propagator. Here's how I did it for a test app:
global_httptextformat = propagators.get_global_httptextformat()
if isinstance(
global_httptextformat, propagators.composite.CompositeHTTPPropagator
) and not any(
isinstance(p, DatadogFormat) for p in global_httptextformat._propagators
):
propagators.set_global_httptextformat(
propagators.composite.CompositeHTTPPropagator(
global_httptextformat._propagators + [DatadogFormat()]
)
)
else:
propagators.set_global_httptextformat(DatadogFormat())
Appending to the global format could be something that's added to the API.
All services would have to have the Datadog propagator added, though that could be part of enabling the Datadog exporter for these services.
@toumorokoshi Any problems you see with that approach?
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.
The composite approach makes sense as a transition, I was referring more toward using open propagation exclusively.
But it sounds like that would work perfectly fine here, the only caveat is the missing DD_ORIGIN propagation, which isn't needed for all the features.
7e9bb8c
to
737c420
Compare
This adds
DatadogFormat
for extracting and injecting trace contexts with Datadog-specific headers.