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: ensure correct run-context for instrumented aws-sdk client usage #2472

Merged
merged 7 commits into from
Dec 2, 2021

Conversation

trentm
Copy link
Member

@trentm trentm commented Nov 25, 2021

This fixes aws-sdk (S3, SQS, SNS) instrumentation to ensure the
automatically created AWS spans do not spill into user code. This also
ensures that any captured APM error for a failed AWS command is a child
of the AWS span.

Refs: #2430

Checklist

  • Implement code
  • Add tests
  • Add CHANGELOG.asciidoc entry

This fixes aws-sdk (S3, SQS, SNS) instrumentation to ensure the
automatically created AWS spans do not spill into user code. This also
ensures that any captured APM error for a failed AWS command is a child
of the AWS span.

Refs: #2430
@trentm trentm requested a review from astorm November 25, 2021 21:09
@trentm trentm self-assigned this Nov 25, 2021
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Nov 25, 2021
@apmmachine
Copy link
Contributor

apmmachine commented Nov 25, 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-12-01T21:36:34.727+0000

  • Duration: 19 min 13 sec

  • Commit: 61706e5

Test stats 🧪

Test Results
Failed 0
Passed 22
Skipped 0
Total 22

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run benchmark tests : Run the benchmark test only.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…spill into user code (#2470)

This fixes the 'http' and 'https' instrumentation for outgoing requests
to not have the 'http' span context be active in user code. This ensures
that user code cannot create a child span of the http span, which would
(a) be misleading and (b) cause problems for coming exit span and
compressed span work.

Also, fix a bug in the https instrumentation in older versions of node
(version <9.0.0) where the instrumentation of `https.request` relied on
intercepting `http.request` (that Node's `https.request()` would call). 
The agent didn't guarantee that the 'http' module was instrumented. 
A user program that used `https.request` without indirectly
`require('http')`ing would not get an HTTP span.

Refs: #2430
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.

👍 Approving -- also, re:

This also
ensures that any captured APM error for a failed AWS command is a child
of the AWS span.

Just to say this a bit more loudly, with the callback bound correct bound, the _setOutcomeFromErrorCapture code is no longer needed captureError will now set this value on the correct span.

@trentm trentm merged commit 9993c6c into master Dec 2, 2021
@trentm trentm deleted the trentm/run-context-aws-sdk branch December 2, 2021 00:11
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.

3 participants