Clarify that a compression-eligible span must not be buffered on an already ended parent #632
Conversation
…lready ended parent Otherwise, if a compression-eligible span ends after its parent has ended, then buffering it on the parent will result in it (or its later sibling) not being reported.
💚 Build Succeeded
Expand to view the summary
Build stats
|
| @@ -172,7 +173,7 @@ void onEnd() { | |||
| } | |||
|
|
|||
| void onChildEnd(Span child) { | |||
| if (!child.isCompressionEligible()) { | |||
| if (ended || !child.isCompressionEligible()) { | |||
There was a problem hiding this comment.
This could be subject to a race condition (check-then-act). What if the parent is ended in a different thread concurrently to this method executing? We could then end up adding the child on a buffer of an already ended span. The child would then never get reported.
Is there a way to avoid this scenario, without having to use locks in onChildEnd?
There was a problem hiding this comment.
I feel like it's acceptable to drop those spans. My thinking is that the scenario where you have this race condition is one of load and in that state there are a lot of spans being reported and any one span missed doesn't significantly affect what gets reported
Otherwise, if a compression-eligible span ends after its parent has
ended, then buffering it on the parent will result in it (or its later
sibling) not being reported.
elastic/apm-agent-nodejs#2623 (review) shows and discusses a Node.js example where a compression-eligible span ends after its parent has ended. (This isn't a far-fetched scenario in Node.js.)
Checklist
sanitize_field_names)CODEOWNERS)