Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: more reliable span.sync determination, drop unused transaction.sync #2326

Merged
merged 10 commits into from
Sep 20, 2021

Conversation

trentm
Copy link
Member

@trentm trentm commented Sep 15, 2021

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 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 asyncHooks
config 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

  • Implement code
  • Add tests
  • Add CHANGELOG.asciidoc entry (waiting on 3.21.0 release first, 3.21.0 #2329)

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
@trentm trentm self-assigned this Sep 15, 2021
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Sep 15, 2021
@apmmachine
Copy link
Contributor

apmmachine commented Sep 15, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-09-20T17:15:10.889+0000

  • Duration: 18 min 26 sec

  • Commit: d512ebb

Test stats 🧪

Test Results
Failed 0
Passed 20
Skipped 0
Total 20

Trends 🧪

Image of Build Times

Image of Tests

…reliability of transaction.test.js usage of agent._transport (CapturingTransport)
Also add a test for span.sync=false for ES and HTTP spans from
@elastic/elasticsearch instrumentation for #1996.

Move the span.sync test from async-hooks.test.js over to span.test.js
because its impl no longer deps on lib/instrumentation/async-hooks.js.

Fixes: #2292
@trentm trentm marked this pull request as ready for review September 15, 2021 21:48
@trentm trentm requested a review from astorm September 15, 2021 21:48
@trentm trentm changed the title fix: more reliable span.sync determination fix: more reliable span.sync determination, drop unused transaction.sync Sep 15, 2021
Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/instrumentation/span.js Show resolved Hide resolved
lib/instrumentation/index.js Show resolved Hide resolved
@trentm trentm requested a review from astorm September 20, 2021 16:12
Copy link
Contributor

@astorm astorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@trentm trentm merged commit c9e4ca2 into master Sep 20, 2021
@trentm trentm deleted the trentm/field-sync branch September 20, 2021 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stop tracking and sending transaction.sync Async spans are being reported as sync
3 participants