-
Notifications
You must be signed in to change notification settings - Fork 421
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
ddtrace/opentelemetry: Introduced OTel name remapping #2337
Conversation
BenchmarksBenchmark execution time: 2023-11-13 15:35:46 Comparing candidate commit 99ab227 in PR branch Found 0 performance improvements and 3 performance regressions! Performance is the same for 36 metrics, 2 unstable metrics. scenario:BenchmarkOTelApiWithCustomTags/otel_api-24
|
I rerun the test suit, including APM_TRACING_E2E_OTEL and failed tests in Parametric, with the system tests branch DataDog/system-tests#1804. All pass |
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.
It would be nice to be able to remove nameSet
and avoid branching if we can.
Co-authored-by: Kyle Nusbaum <kyle@datadog.com>
ddtrace/opentelemetry/span.go
Outdated
for k, v := range s.attributes { | ||
// if we find operation.name, | ||
if k == operationNameKey || k == ext.SpanName { | ||
// set it and keep track that it was set to ignore everything else | ||
if name, ok := v.(string); ok { | ||
s.attributes[ext.SpanName] = strings.ToLower(name) | ||
} | ||
} | ||
} |
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.
Refactoring suggestion:
Similar to the Java pseudocode in the doc, you can just check for an operation.name
attribute in the converter function, and return out early if it exists.
If you do that, then this entire loop can disappear.
i.e. the only changes you need in this entire function then are:
s.SetName(s.createOperationName())
for k, v := range s.attributes {
s.SetTag(k, v)
}
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.
Sorry, what do you mean by converter function
?
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.
By "converter function" I meant the createOperationName
method you have.
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.
Just two last questions, thanks!
What does this PR do?
This PR adds calculating of the operation name based on the OTel attributes and span kind.
Motivation
The OTel API only has the concept of a single name associated with a span, which in Datadog is represented as the “resource name”. However, Datadog has an additional name, the span’s “operation name”, which serves important functions such as metrics calculation, which is used for customer monitoring and alerting setups. Since the OTel API doesn’t have an equivalent for an operation name, we must calculate one.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!