-
Notifications
You must be signed in to change notification settings - Fork 186
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
log discarded span attributes, events, links #1336
log discarded span attributes, events, links #1336
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1336 +/- ##
============================================
+ Coverage 74.38% 74.58% +0.19%
- Complexity 2493 2504 +11
============================================
Files 354 355 +1
Lines 7144 7180 +36
============================================
+ Hits 5314 5355 +41
+ Misses 1830 1825 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
moving the warning removes an extra call to toSpanData, thus some overhead. Outside of tests, this function is only called from span processors once
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.
Would it perhaps make sense to skip logging for not sampled spans?
IMO we should log discarded data for all recorded spans, not only for sampled spans. Usage of a sampler that records a span indicates that a user is interested in the span data (the built-in samplers return DROP or RECORD_AND_SAMPLE, not RECORD_ONLY).
Outside of tests, this function is only called from span processors once
Every span processor calls ::toSpanData()
-> currently would be logged multiple times if multiple span processors are registered.
(We'll need the same for LogRecord
attributes spec)
src/SDK/Trace/Span.php
Outdated
@@ -290,6 +292,18 @@ public function toSpanData(): SpanDataInterface | |||
$this->endEpochNanos, | |||
$this->hasEnded | |||
); | |||
|
|||
if ($spanData->getTotalDroppedLinks() || $spanData->getTotalDroppedEvents() || $spanData->getAttributes()->getDroppedAttributesCount()) { |
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.
Should also check for dropped event or link attributes.
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.
Updated to check+log on LogRecord dropped attributes, span event+link dropped attributes.
Moved the check into span's onEnd
, so that it is called once per span. This does add an extra call to toSpanData
. I didn't profile the difference, but it looks like a computationally light function...
The SDK should log once if there are any dropped span attributes, links or events.
The SDK should log once if there are any dropped LogRecord attributes.
Fixes: #1323