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

instrumentation of @aws-sdk/client-dynamodb #3486

Closed
wants to merge 20 commits into from

Conversation

david-luna
Copy link
Member

@david-luna david-luna commented Jul 11, 2023

PR to add instrumentation to @aws-sdk/client-dynamodb package. Since in SDK v3 all clients are similar I took a same approach done in S3 intrumentation

  • a soft guard is added for different spans triggering the middleware
  • a guard is added is case finalizeRequest middleware is not executed

Closes #2958

Checklist

  • Implement code
  • Add tests
  • Update TypeScript typings
  • Update documentation
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jul 11, 2023
@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Jul 11, 2023
* Slurp everything from the given ReadableStream and return the content,
* converted to a string.
*/
async function slurpStream (stream) {
Copy link
Member Author

@david-luna david-luna Jul 11, 2023

Choose a reason for hiding this comment

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

Note for reviewer: function copied here to be used in use-client-* files. Current dynamodb tests don't make use of it, yet.

@david-luna david-luna requested a review from trentm July 17, 2023 12:51
@david-luna
Copy link
Member Author

david-luna commented Jul 17, 2023

Ci failing due to a problem in environment. Apparently we're using a newer version of npm with Nodejs <=14

ERROR: npm v9.8.0 is known not to run on Node.js v12.0.0.  This version of npm supports the following node versions: `^14.17.0 || ^16.13.0 || >=18.0.0`. You can find the latest version at [https://nodejs.org/.](https://nodejs.org/)

This does not make much sense with the previous message shown by the GH action setup-node@v3 which shows

Run actions/setup-node@v3
  with:
    node-version: 14.0
    always-auth: false
    check-latest: false
    token: ***

Attempting to download 14.0...
Acquiring 14.0.0 - x64 from https://github.com/actions/node-versions/releases/download/14.0.0-20200507.99/node-14.0.0-linux-x64.tar.gz
Extracting ...
/usr/bin/tar xz --strip 1 --warning=no-unknown-keyword -C /home/runner/work/_temp/de6b1539-a95c-46f0-9478-d114e0041d85 -f /home/runner/work/_temp/a219392d-3962-4c6f-9873-9a0f313a7c49
Adding to the cache ...

Environment details
  node: v14.0.0
  npm: 6.14.4
  yarn: 1.22.19

Link to the error: https://github.com/elastic/apm-agent-nodejs/actions/runs/5575905829/jobs/10187134325

@david-luna david-luna marked this pull request as ready for review July 17, 2023 13:49
Copy link
Member Author

@david-luna david-luna left a comment

Choose a reason for hiding this comment

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

/test tav @aws-sdk/client-dynamodb

@david-luna
Copy link
Member Author

Ci failing due to a problem in environment. Apparently we're using a newer version of npm with Nodejs <=14

ERROR: npm v9.8.0 is known not to run on Node.js v12.0.0.  This version of npm supports the following node versions: `^14.17.0 || ^16.13.0 || >=18.0.0`. You can find the latest version at [https://nodejs.org/.](https://nodejs.org/)

This does not make much sense with the previous message shown by the GH action setup-node@v3 which shows

Run actions/setup-node@v3
  with:
    node-version: 14.0
    always-auth: false
    check-latest: false
    token: ***

Attempting to download 14.0...
Acquiring 14.0.0 - x64 from https://github.com/actions/node-versions/releases/download/14.0.0-20200507.99/node-14.0.0-linux-x64.tar.gz
Extracting ...
/usr/bin/tar xz --strip 1 --warning=no-unknown-keyword -C /home/runner/work/_temp/de6b1539-a95c-46f0-9478-d114e0041d85 -f /home/runner/work/_temp/a219392d-3962-4c6f-9873-9a0f313a7c49
Adding to the cache ...

Environment details
  node: v14.0.0
  npm: 6.14.4
  yarn: 1.22.19

Link to the error: https://github.com/elastic/apm-agent-nodejs/actions/runs/5575905829/jobs/10187134325

Mystery solved, npm leaked into this PR's package.json although I do not remember adding it as a dependency

@trentm
Copy link
Member

trentm commented Jul 31, 2023

Sorry for your merge pain. Might be easiest to do a merge like this:

  • copy in the state of the file on this PR before re-merging with main
  • run npm run lint:fix to update its styling
  • git add ... that file

@david-luna
Copy link
Member Author

Closing this PR to prepare this instrumentation for next major release

@david-luna david-luna closed this Aug 1, 2023
APM-Agents (OLD) automation moved this from In Progress to Done Aug 1, 2023
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
Development

Successfully merging this pull request may close these issues.

AWS instrumentation for SDK v3, DynamoDB
2 participants