fix: more reliable span.sync determination, drop unused transaction.sync#2326
Merged
fix: more reliable span.sync determination, drop unused transaction.sync#2326
Conversation
Current `span.sync` is tracked via: - the "before" async hook setting it false on the "active" span, and - Instrumentation#bindFunction's wrapper setting it false, on the fair assumption that all usages of bindFunction are for async callbacks. The former has issues when there are multiple active spans within a single async task -- as is the case with Elasticsearch instrumentation (issue #1996) and the aws-sdk instrumentations (which have manual workarounds). This changes to set sync=false if the executionAsyncId() at end-time is different than at start-time. This works for whatever `asyncHooks` config var value. Fixes: #1996
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
…reliability of transaction.test.js usage of agent._transport (CapturingTransport)
astorm
reviewed
Sep 16, 2021
Contributor
astorm
left a comment
There was a problem hiding this comment.
Neat! (and smart!) -- overall I like this change and this seems like a better/more-accurate way of tracking whether a span has cross an asynchronous boundary (I'm increasingly skeptical of how useful this is to track for the general Node.js user -- but that's a separate issue :))
Happy to get on the approval train once the perf. and patch-async questions are worked through.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current
span.syncis tracked via:fair assumption that all usages of bindFunction are for async
callbacks.
The former has issues when there are multiple active spans within a
single async task -- as is the case with Elasticsearch instrumentation
(issue Async spans are being reported as sync #1996) and the aws-sdk instrumentations (which have manual
workarounds).
This changes to set sync=false if the executionAsyncId() at end-time
is different than at start-time. This works for whatever
asyncHooksconfig var value.
One change in behaviour is that the value of span.sync is only
guaranteed to be accurate after span.end() is called. Given that
span.sync isn't a public field, I believe this is fine.
This change also drops the 'transaction.sync' fields. It was never
used by APM server.
Fixes: #1996
Fixes: #2292
Checklist