feat: add @aws-sdk/client-dynamodb instrumentation #3530

david-luna · 2023-08-01T12:07:29Z

Adding@aws-sdk/client-dynamodb v3 instrumentation and plugin it in @smithy/smithy-client one. The tests have been created taking v2 instrumentation tests as a template.

Closes #2958

Checklist

david-luna · 2023-08-01T12:12:06Z

.ci/tav.json

@@ -4,6 +4,7 @@
  "modules": [
    "@apollo/server",
    "@aws-sdk/client-s3",
+    "@aws-sdk/client-dynamodb",


Note for reviewer: still under the limit and I guess the matrix will be reduced once we drop older node versions.

david-luna · 2023-08-01T12:15:08Z

test/_utils.js

+ * Slurp everything from the given ReadableStream and return the content,
+ * converted to a string.
+ */
+async function slurpStream(stream) {


Note for reviewer: moved this function to a common place for further use in other client tests

trentm

Nice work. This is great.

lib/instrumentation/modules/@aws-sdk/client-dynamodb.js

test/instrumentation/modules/@aws-sdk/client-dynamodb.test.js

.ci/scripts/test.sh

.tav.yml

david-luna · 2023-09-14T10:38:52Z

test/instrumentation/modules/@aws-sdk/client-dynamodb.test.js

+
+const NODE_VER_SUP_AWS_SDK = '14.0.0';
+const NODE_VER_SUP_RANGE_RITM = `>=${NODE_VER_SUP_AWS_SDK}`;
+const NODE_VER_SUP_RANGE_IITM = iitmVersionsFrom(NODE_VER_SUP_AWS_SDK);


Note for reviewer: this is to avoid testing ESM on Node v12 since the dynamodb client doesn't support it. Check ./test/testconsts.js module for implementation details.

nit: I'm fine with this, but I feel it is a lot of code that is ultimate less clear than a second constant, something like:

const NODE_VER_RANGE_IITM_GE14 = '^14.13.1 || ^16.0.0 || ^18.1.0 <20'; // NODE_VER_RANGE_IITM minus node v12

Then perhaps these other usages could be updated:

test/instrumentation/modules/fastify/fastify.test.js 64: node: '^14.13.1 || ^16.0.0 || ^18.1.0 <20', // NODE_VER_RANGE_IITM minus node v12 because top-level `await` is used test/instrumentation/modules/http/http-esm.test.js 111: node: '^14.13.1 || ^16.0.0 || ^18.1.0 <20', // NODE_VER_RANGE_IITM minus node v12 because top-level `await` is used 137: node: '^14.13.1 || ^16.0.0 || ^18.1.0 <20', // NODE_VER_RANGE_IITM minus node v12 because top-level `await` is used

done! also did the replacement in fastify and http tests :)

david-luna

A probably spurious test in apm-http-client

# payloadLogFile
ok 40 should have received 5 objects
not ok 41 plan != count
  ---
    operator: fail
    expected: 6
    actual:   1
    at: Transform.<anonymous> (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/apm-client/http-apm-client/config.test.js:572:13)
    stack: |-
      Error: plan != count
          at Test.assert [as _assert] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:309:48)
          at Test.fail (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:403:7)
          at completeEnd (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:224:9)
          at next (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:234:4)
          at Test._end (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:254:2)
          at Test.end (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:194:7)
          at Transform.<anonymous> (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/apm-client/http-apm-client/config.test.js:572:13)
          at Transform.emit (node:events:514:28)
          at endReadableNT (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/readable-stream/lib/_stream_readable.js:1003:12)
          at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
  ...
# update conf

.ci/scripts/test.sh

trentm · 2023-09-14T21:29:10Z

test/instrumentation/modules/@aws-sdk/client-dynamodb.test.js

+
+const NODE_VER_SUP_AWS_SDK = '14.0.0';
+const NODE_VER_SUP_RANGE_RITM = `>=${NODE_VER_SUP_AWS_SDK}`;
+const NODE_VER_SUP_RANGE_IITM = iitmVersionsFrom(NODE_VER_SUP_AWS_SDK);


nit: I'm fine with this, but I feel it is a lot of code that is ultimate less clear than a second constant, something like:

const NODE_VER_RANGE_IITM_GE14 = '^14.13.1 || ^16.0.0 || ^18.1.0 <20'; // NODE_VER_RANGE_IITM minus node v12

Then perhaps these other usages could be updated:

test/instrumentation/modules/fastify/fastify.test.js 64: node: '^14.13.1 || ^16.0.0 || ^18.1.0 <20', // NODE_VER_RANGE_IITM minus node v12 because top-level `await` is used test/instrumentation/modules/http/http-esm.test.js 111: node: '^14.13.1 || ^16.0.0 || ^18.1.0 <20', // NODE_VER_RANGE_IITM minus node v12 because top-level `await` is used 137: node: '^14.13.1 || ^16.0.0 || ^18.1.0 <20', // NODE_VER_RANGE_IITM minus node v12 because top-level `await` is used

test/instrumentation/modules/@aws-sdk/fixtures/use-client-dynamodb.js

CHANGELOG.asciidoc

lib/instrumentation/modules/@aws-sdk/client-dynamodb.js

trentm · 2023-09-14T22:17:06Z

^^ Basically all LGTM. Just the changelog entry move... and with "dismiss stale reviews", no point in an "approve".

…modb.js Co-authored-by: Trent Mick <[email protected]>

Co-authored-by: Trent Mick <[email protected]>

david-luna added 24 commits July 11, 2023 18:21

feat: add dynamodb v3 instrumentation

b5d4220

chore: add tests for dynamodb v3

2323ba0

chore: update comments

086a810

chore: update requires

85bf88f

chore: add missing tests

9a2c0e4

docs: update docs

8504fbb

chore: update TAV

0ec8f76

chore: update test scripts

f89c76e

Merge branch 'main' into dluna/2958-aws-dynamodb-v3-instr

fc310dc

chore: fix test.sh

b068570

chore: fix changelog

2b9a826

chore: hold on OTel attributes

4735d0a

chore: add sample ouput gist

7bc5d97

chore: revert test.sh

4457c15

chore: add script logs

fb886f4

chore: add script logs

e5d1142

chore: add script logs

2962d5e

chore: update dependencies

e8446f5

chore: aling lock file with main

8c888dc

chore: update lock file

d1c3fe0

chore: merge main branch

66b423e

chore: add lock dynamodb dependency

cdd7637

chore: lint fix dynamodb files

8240192

chore: add ESM tests

8933b46

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Aug 1, 2023

david-luna changed the base branch from main to dev/4.x August 1, 2023 12:09

david-luna commented Aug 1, 2023

View reviewed changes

chore: fix linting issues

2f296ec

david-luna marked this pull request as draft August 1, 2023 18:09

trentm previously approved these changes Aug 1, 2023

View reviewed changes

lib/instrumentation/modules/@aws-sdk/client-dynamodb.js Show resolved Hide resolved

test/instrumentation/modules/@aws-sdk/client-dynamodb.test.js Outdated Show resolved Hide resolved

david-luna added 6 commits September 6, 2023 10:58

chore: merge main branch

fe33e08

chore: add dynamodb into tav.json

fad971f

chore: update nede version check in tests

c0543b8

chore: fix lint

6dbd591

chore: change iitm range computation

6329b53

tests: better range calculation

3843b41

david-luna dismissed trentm’s stale review via 3843b41 September 13, 2023 11:22

david-luna added 2 commits September 14, 2023 11:53

Merge branch 'main' into dluna/2958-aws-dynamodb-v3-instr

3dbac43

chore: update tav versions for S3 and dynamodb

f917437

david-luna marked this pull request as ready for review September 14, 2023 10:24

david-luna commented Sep 14, 2023

View reviewed changes

.ci/scripts/test.sh Show resolved Hide resolved

david-luna commented Sep 14, 2023

View reviewed changes

.tav.yml Show resolved Hide resolved

david-luna commented Sep 14, 2023

View reviewed changes

chore: remove logs

21fc7cf

david-luna requested a review from trentm September 14, 2023 10:45

david-luna commented Sep 14, 2023

View reviewed changes

trentm reviewed Sep 14, 2023

View reviewed changes

david-luna and others added 4 commits September 18, 2023 10:28

Update test/instrumentation/modules/@aws-sdk/fixtures/use-client-dyna…

2eebdfa

…modb.js Co-authored-by: Trent Mick <[email protected]>

Update lib/instrumentation/modules/@aws-sdk/client-dynamodb.js

e0511cc

Co-authored-by: Trent Mick <[email protected]>

tests: add new IITM range constant

3b64c75

chore: lit fix

7879891

david-luna requested a review from trentm September 18, 2023 15:10

trentm approved these changes Sep 18, 2023

View reviewed changes

david-luna merged commit 3adbadf into main Sep 18, 2023
14 checks passed

david-luna deleted the dluna/2958-aws-dynamodb-v3-instr branch September 18, 2023 17:37

fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024

feat: add @aws-sdk/client-dynamodb instrumentation (elastic#3530)

a057354

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add @aws-sdk/client-dynamodb instrumentation #3530

feat: add @aws-sdk/client-dynamodb instrumentation #3530

david-luna commented Aug 1, 2023 •

edited

Loading

david-luna Aug 1, 2023

david-luna Aug 1, 2023

trentm left a comment

david-luna Sep 14, 2023

trentm Sep 14, 2023

david-luna Sep 18, 2023

david-luna left a comment •

edited

Loading

trentm Sep 14, 2023

trentm commented Sep 14, 2023

feat: add @aws-sdk/client-dynamodb instrumentation #3530

feat: add @aws-sdk/client-dynamodb instrumentation #3530

Conversation

david-luna commented Aug 1, 2023 • edited Loading

Checklist

david-luna Aug 1, 2023

Choose a reason for hiding this comment

david-luna Aug 1, 2023

Choose a reason for hiding this comment

trentm left a comment

Choose a reason for hiding this comment

david-luna Sep 14, 2023

Choose a reason for hiding this comment

trentm Sep 14, 2023

Choose a reason for hiding this comment

david-luna Sep 18, 2023

Choose a reason for hiding this comment

david-luna left a comment • edited Loading

Choose a reason for hiding this comment

trentm Sep 14, 2023

Choose a reason for hiding this comment

trentm commented Sep 14, 2023

david-luna commented Aug 1, 2023 •

edited

Loading

david-luna left a comment •

edited

Loading