-
Notifications
You must be signed in to change notification settings - Fork 314
Normalizing Behavior of OTel Spans Created From Custom-Instrumentation and Trace Annotations #9759
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
private static String convertToSpanType(SpanKind kind) { | ||
if (kind == null) { | ||
return null; | ||
} | ||
switch (kind) { | ||
case SERVER: | ||
return HTTP_SERVER; | ||
case CLIENT: | ||
return HTTP_CLIENT; | ||
case PRODUCER: | ||
return MESSAGE_PRODUCER; | ||
case CONSUMER: | ||
return MESSAGE_CONSUMER; | ||
case INTERNAL: | ||
// checking for SpanKind.INTERNAL, returning DDSpanTypes.INTERNAL | ||
return INTERNAL; | ||
default: | ||
return null; | ||
} | ||
} | ||
|
||
private static String convertToSpanKindTag(SpanKind kind) { | ||
if (kind == null) { | ||
return null; | ||
} | ||
switch (kind) { | ||
case CLIENT: | ||
return SPAN_KIND_CLIENT; | ||
case SERVER: | ||
return SPAN_KIND_SERVER; | ||
case PRODUCER: | ||
return SPAN_KIND_PRODUCER; | ||
case CONSUMER: | ||
return SPAN_KIND_CONSUMER; | ||
case INTERNAL: | ||
return SPAN_KIND_INTERNAL; | ||
default: | ||
return null; | ||
} |
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 currently duplicated code between the opentelemetry-shim
and opentelemetry-annotation
modules. @PerfectSlayer what do you think about introducing a new opentelemetry-utils
module in utils
for shared helpers? 🤔
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.
Note that the otel-shim
classes are already duplicated at build-time and placed under different packages - i.e. one source, but made available in two places in the final jar. This is to support both OTel drop-in support (which cannot expose the OTel packages on the classpath, so must repackage classes both at build-time and run-time) but also instrument OTel client code in the application.
I would investigate whether the annotations instrumentation can also depend on
implementation project(':dd-java-agent:agent-otel:otel-shim')
like the main OTel instrumentation under opentelemetry-1.4
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.
❔ question: My main question for this change is should we change this behavior as it is not part of the spec, or should we evolve the spec and make sure the behavior is consistent across all languages? -- system tests could help here too.
Because I can think of case where customers set span type on purpose and it gets overridden when setting span kind.
public static final String CACHE = "cache"; | ||
public static final String SOAP = "soap"; | ||
|
||
public static final String INTERNAL = "internal"; |
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.
❔ question: It looks like a duplicate of datadog.trace.bootstrap.instrumentation.api.Tags#SPAN_KIND_INTERNAL
* @param kind The OpenTelemetry span kind to convert. | ||
* @return The {@link DDSpanTypes} value. | ||
*/ | ||
public static String convertToSpanType(SpanKind kind) { |
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.
🎯 suggestion: To keep method naming consistent:
public static String convertToSpanType(SpanKind kind) { | |
public static String toSpanType(SpanKind kind) { |
if (kind == null) { | ||
return null; | ||
} |
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.
🎯 suggestion: null
is not supposed to be given as parameter as all calls are guarded.
Similarly, I would not have a default case returning null
but internal.
I think you are right that this change deviates from the spec. There are 2 concepts that are closely tied together, which makes this situation confusing. Below are the changes I think actually should be done:
I looked into the OTel implementation of using a What do you think @PerfectSlayer? I can see a world where we should keep the legacy behavior of |
No I think it make sens to change to support both span kind and span type on OTel Tracing API and annotation support. One thing that might be missing is the implementation of the |
What Does This Do
Currently, OTel Spans created from Trace Annotations do not contain the
span.kind
tag, and spans created from custom instrumentation do not have thetype
field. This leads to an inconsistent customer experience where spans are missing data on the backend, and also may have implications on the way that spans are "named" following normalization on the Agent. This PR aims to reduce the inconsistent behavior between these two ways of creating OTel spans.Motivation
Escalation
Additional Notes
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any useful labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]