feat(sdk-trace-base): add diagnostic logging when spans are dropped #3610
Conversation
neoeinstein
force-pushed
the
dropped-span-reporting
branch
from
67b88c6
to
ddb2cf7
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3610 +/- ##
==========================================
+ Coverage 94.01% 94.03% +0.01%
==========================================
Files 268 268
Lines 7920 7927 +7
Branches 1641 1643 +2
==========================================
+ Hits 7446 7454 +8
+ Misses 474 473 -1
|
|
Is it possible to add a test for this? |
neoeinstein
force-pushed
the
dropped-span-reporting
branch
from
f8423bd
to
008255b
Compare
|
Updated with a commit that includes a test for this behavior. |
| if (this._droppedSpansCount === 0) { | ||
| diag.debug('maxQueueSize reached, dropping spans'); | ||
| } | ||
| this._droppedSpansCount++; |
There was a problem hiding this comment.
This will be very chatty in a system overloaded enough to be dropping spans. Do you think it's needed when the warn log on line 132 is already doing this job?
There was a problem hiding this comment.
For a given stretch of dropped spans, this message will only be reported once, just like the summary warning, so should have the same cardinality as that one. This debug diagnostic can be used to identify at what time those drops started. It's recorded as debug, which should be able to be filtered out if debug diagnostics aren't interesting or desired.
I found it useful as there are two knobs that can be adjusted to prevent dropping spans: queue size and the export interval. The warning message makes it easier to adjust the queue size, but the debug message can make it apparent how often one should be exporting if they don't want to carry a larger memory load associated with a larger queue.
I'm not particularly attached to this message, so if we want it removed, that's fine with me.
Which problem is this PR solving?
The current JS SDK does not provide any sort of indication that the batch span processor is dropping spans, resulting in difficult to identify missing spans.
Short description of the changes
Adds diagnostic logging when spans begin and stop being dropped. When spans are being dropped, a debug diagnostic is logged indicating that spans are now being dropped, and a count is maintained. When a span is received that is successfully enqueued, the number of spans that were dropped between successful enqueue attempts is reported as a warning diagnostic and the count is reset to 0.
An alternate would be to add a private event emitter that would emit an event any time a span were dropped, allowing the consumer to subscribe and perform whatever action they deemed appropriate in reaction. However, this would require changing the publicly exposed API, so the diagnostic logging mechanism is utilized instead.
Type of change
How Has This Been Tested?
No new tests added, but verified that existing tests continue to pass with expected behavior. This change only adds diagnostic logging with minimal logic to prevent flooding diagnostic logs, which is not externally visible.
Checklist: