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.

Sign up for GitHub

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

Jump to bottom

stop tracking and sending transaction.sync #2292

Closed
trentm opened this issue Aug 19, 2021 · 1 comment · Fixed by #2326
Closed

stop tracking and sending transaction.sync #2292

trentm opened this issue Aug 19, 2021 · 1 comment · Fixed by #2326
Assignees
@trentm
Labels
agent-nodejs Make available for APM Agents project planning.

Comments

@trentm
Copy link
Member

trentm commented Aug 19, 2021

The APM server intake model does not include it: https://github.com/elastic/apm-server/blob/2c65c70cda74aa3eb4633c022417548ef52d89c7/model/modeldecoder/v2/model.go#L797-L846
It is only defined for spans: https://github.com/elastic/apm-server/blob/2c65c70cda74aa3eb4633c022417548ef52d89c7/model/modeldecoder/v2/model.go#L647-L648

The current Node.js agent is:
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Aug 19, 2021
@trentm
Copy link
Member Author

trentm commented Aug 19, 2021

@astorm Do you recall why you split this test case in test/instrumentation/async-hooks.js into two separate tests cases?
https://github.com/elastic/apm-agent-nodejs/pull/2081/files#diff-7452104585e564b9dc20f0b64aa156c0701046c833513e01aaf2c55a1f172260

@trentm trentm mentioned this issue Sep 15, 2021
Merged
3 tasks
@trentm trentm self-assigned this Sep 15, 2021
trentm added a commit that referenced this issue Sep 15, 2021
@trentm
fix: drop 'transaction.sync' field, it was never used by apm-server
39e3854 
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 added a commit that referenced this issue Sep 20, 2021
@trentm
fix: more reliable span.sync determination, drop unused transaction.s…
c9e4ca2 
…ync (#2326)

Currently `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 decouples from async context
management, so works for whatever value of the `asyncHooks`
config var.

This also drops the `transaction.sync` field which was never used.

Fixes: #1996
Fixes: #2292
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@trentm trentm

Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: more reliable span.sync determination, drop unused transaction.sync elastic/apm-agent-nodejs
1 participant
@trentm