fix(span): Do not pass stack frames into promises #269
Conversation
|
I think the reason why the tests are failing is the same as I discovered when I originally moved the encoding to before the transactions were added to the queue (see background below). Besides fixing the tests, it would be preferable to find a way to deal with the new state a transaction can be in: Ended, but not yet added to the queue. If a user calls Alternatively, we move the I/O part of the encoding to flush time. This means that the first part of the encoding that happens before the transactions are added to the queue becomes synchronously. Come to think of it, I actually think this is the most elegant solution, as it doesn't add any extra state to the agent. BackgroundBefore we used promises for this, when a transaction ended, it was added to the queue synchronously. When the queue needed flushing, we looped over all the transaction objects in it and called their The Promise got added when we decided to do the encoding of the transaction up front and add the simpler object to the queue instead. Reason being that the encoding of transactions is an async operation as it does I/O. If we didn’t add a Promise to the queue in that case, we would have a mid state where a transaction was actually ended, but not yet added to the queue. This would make testing a lot harder as we would not be able to ask questions like, “when 10 transactions have ended, there should be 10 transactions in the queue” etc. It would also mean that we wouldn’t be able to flush the queue on process exit and be sure all transactions safely made it to the server. |
Qard
changed the title
Qard
force-pushed
the
no-promise-queue
branch
2 times, most recently
from
44beee7
to
bee97fb
Compare
lib/instrumentation/span.js
Outdated
| parsers.parseCallsite(callsite, false, self._agent._conf, next()) | ||
| }) | ||
| }) | ||
| this._stackObj.then( |
There was a problem hiding this comment.
Today this._stackObj will be null if captureSpanStackTraces is false, in which case this is safe. But as this is a pretty loose relationship, I wonder if we should add a guard to ensure we can't get to here unless this._stackObj is set. Just to make it more future-proof. What do you think?
There was a problem hiding this comment.
It might just be me that's overly cautious 😅
Passing error stack frames into a promise context makes it uncollectable by the garbage collector.
I replaced the code that queues and waits for promises with something that just defers the queue until after the encode. The excessive promise usage had some noticeable overhead.
This may result in transactions entering the queue out of order, but I don't think that matters.