[Tracer] Adds SpanEvents Support for DD and OTEL #2754
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2754 +/- ##
============================================
- Coverage 80.74% 78.47% -2.27%
- Complexity 2452 2463 +11
============================================
Files 144 170 +26
Lines 14344 18190 +3846
Branches 0 913 +913
============================================
+ Hits 11582 14275 +2693
- Misses 2762 3420 +658
- Partials 0 495 +495
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 26 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
BenchmarksBenchmark execution time: 2024-08-19 16:38:41 Comparing candidate commit aadf273 in PR branch Found 0 performance improvements and 4 performance regressions! Performance is the same for 174 metrics, 0 unstable metrics. scenario:LaravelBench/benchLaravelOverhead-opcache
scenario:PDOBench/benchPDOBaseline-opcache
scenario:PDOBench/benchPDOOverhead-opcache
scenario:TraceSerializationBench/benchSerializeTrace
|
| // Update span metadata based on exception attributes | ||
| $this->setAttribute(Tag::ERROR_MSG, $exceptionAttributes['exception.message']); | ||
| $this->setAttribute(Tag::ERROR_TYPE, $exceptionAttributes['exception.type']); | ||
| $this->setAttribute(Tag::ERROR_STACK, $exceptionAttributes['exception.stacktrace']); |
There was a problem hiding this comment.
Are you sure about this? Exceptions generally should only be added on spans if they escape their span.
There was a problem hiding this comment.
This is by design, to match what our Datadog Agent does for OTEL Events when it receives OTLP traces. If there are any exception events on a span, then the last event that is added will be used to populate the error tagging fields on the span.
Note: We do not update the span.Error field when calling this method. To update the span.Error field, the user would have to call the OpenTelemetry span.SetStatus API with the Error status.
There was a problem hiding this comment.
error is set automatically when error.message or error.type are present. So, at least that assumption is not correct here.
When manually adding errors, users are supposed to only add error.message and error = 1 is automatically added for them.
There was a problem hiding this comment.
Also IIRC that behaviour of populating error.message is a fallback behaviour from the ages where no span event support existed in the UI.
There was a problem hiding this comment.
Hey @bwoebi, I've been looking into this ask here since I thought for us to work around this I could check if the span was an error or not, if not then reset the error tag but I am not sure how to accomplish this, for example I saw that the error can be removed from the metrics object using unset($this->span->metrics[Tag::ERROR]); but couldn't find a way to do so for the body of the Span and wanted to check what you'd recommend.
There was a problem hiding this comment.
What I would recommend is not doing this, honestly.
Like, it's a bad idea which goes against the defined semantics of the tracer. In the PHP tracer, error_msg/error_type is reserved for exceptions escaping the span.
There was a problem hiding this comment.
I see what you mean, will go ahead with only adding the stack tag for now and later connect with Error Tracking to try getting the details from the span event instead, does that sounds better at the moment?
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
bwoebi
force-pushed
the
maximo/span-events-support
branch
from
257b6f1
to
853e555
Compare
This reverts commit 4d316e4.
ext/ddtrace.c
Outdated
| } | ||
| } | ||
|
|
||
| if (attributes && Z_TYPE_P(attributes) == IS_ARRAY) { |
There was a problem hiding this comment.
| if (attributes && Z_TYPE_P(attributes) == IS_ARRAY) { | |
| if (Z_TYPE_P(attributes) == IS_ARRAY) { |
Nit: redundant.
src/DDTrace/OpenTelemetry/Span.php
Outdated
| $event = new Event( | ||
| $datadogSpanEvent->name, | ||
| (int)$datadogSpanEvent->timestamp, | ||
| Attributes::create((array)$datadogSpanEvent->attributes ?? []) |
There was a problem hiding this comment.
This creates an OTel-Event, but it won't include the exception. Can you generate the right exception-attributes here on the fly? if $datadogSpanEvent instanceof ExceptionSpanEvent? (i.e. exception.stacktrace, exception.type, exception.message)?
There was a problem hiding this comment.
I.e. same than what you do in SpanBuilder::recordException
|
I see you updated libdatadog. Can you please also run |
Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
…span-events-support
Description
Adding support for SpanEvents, related RFC, currently, added a SpanEvent class and ExceptionSpanEvent which inherits from it to add specialized exception events to the Span.
Related PRs:
[PHP] Enabling SpanEvents Tests system-tests#2841
Reviewer checklist