fix(opentelemetry): Ensure only orphaned spans of sent spans are sent #16590
Conversation
| @@ -192,6 +189,7 @@ export class SentrySpanExporter { | |||
| ); | |||
|
|
|||
| for (const span of sentSpans) { | |||
| this._sentSpans.set(span.spanContext().spanId, Date.now() + DEFAULT_TIMEOUT * 1000); | |||
There was a problem hiding this comment.
Just noticed, we should probably move out the Date.now() + DEFAULT_TIMEOUT * 1000 part to avoid recalculating it over the loop.
There was a problem hiding this comment.
ah, great catch too! 👍 will do!
There was a problem hiding this comment.
Good catch! That said I think this was intentional as sometimes we never send spans so they don't end up in sentSpans. We may rename this._sentSpans to this._finishedSpans to avoid the confusion. To avoid going over this list a second time, we may try to bake the logic into this._maybeSend I guess. Not sure how impactful that would be tho.
|
so we def. cannot do this based on finished spans, as that breaks the product quite drastically - as generally, it is really bad if we send spans with the wrong transaction (as the product still relies on this a lot), so this should only be last resort. This is leading to bugs today (where e.g. db spans show up as transactions instead of as spans of their http.server transaction how they should be) 😬 |
|
I also added a test that previously failed - which shows that if the actual root span is not sent yet, we should never send the child span on it's own, as that breaks the product. |
mydea
changed the title
mydea
force-pushed
the
fn/fix-spanexporter-bug
branch
from
a65df3d to
024562c
Compare
if that happens IMHO this is a major problem that should be fixed at its root rather than working around it like this 🤔 |
| @@ -3,16 +3,15 @@ import { waitForTransaction } from '@sentry-internal/test-utils'; | |||
|
|
|||
| test('should create AI spans with correct attributes', async ({ page }) => { | |||
| const aiTransactionPromise = waitForTransaction('nextjs-15', async transactionEvent => { | |||
| return transactionEvent?.transaction === 'ai-test'; | |||
| return transactionEvent.transaction === 'GET /ai-test'; | |||
There was a problem hiding this comment.
this actually also shows the incorrect behavior we had before - the ai-test span was sent as a transaction instead of as part of the GET /ai-test span, which is what it should have been.
Following up on this to prevent FUD: I think this was due to those spans belonging to root spans that were already sent (so the very thing the patch was aiming to fix). That means the current behavior (the one with your fix in) is 100% correct and there shouldn't be any spans not being sent or not being considered to be sent. TL;DR -- My earlier comment was not fully accurate and I don't think we need anything more to investigate regarding that. Sorry for the confusion. |
Noticed this small but potentially impactful typo: We used to mark all finished spans as sent, not just the ones that are actually sent :O (Also we iterated twice over the same array, streamlined this by just doing this once).