Skip to content

feat: add S3 Bucket and Key params in instrumentation #3274

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
Merged
merged 6 commits into from
Apr 20, 2023
Merged

feat: add S3 Bucket and Key params in instrumentation #3274

merged 6 commits into from
Apr 20, 2023

Conversation

david-luna
Copy link
Member

@david-luna david-luna commented Apr 18, 2023 •
edited by trentm
Loading

Extract the parameters Bucket and Key from the instrumented requests to S3. Parameters are added into OTel attributes of the span following the spec

Checklist

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

Closes: #3150

@david-luna david-luna requested a review from trentm April 18, 2023 11:49
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Apr 18, 2023
@david-luna david-luna marked this pull request as draft April 18, 2023 11:49
trentm
trentm requested changes Apr 19, 2023
View reviewed changes
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

This is looking great. Some comments below.

@cla-checker-service
Copy link

cla-checker-service bot commented Apr 19, 2023 •
edited
Loading

💚 CLA has been signed

trentm
trentm approved these changes Apr 19, 2023
View reviewed changes
lib/instrumentation/modules/aws-sdk/s3.js Outdated
@@ -65,6 +67,16 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version,
return orig.apply(request, origArguments)
}

const otelAttrs = span._getOTelAttributes()
Copy link
Member

Choose a reason for hiding this comment

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

Oh, I misunderstood you when we discussed this earlier. I hadn't realized you were proposing dropping the if (bucket) { ... } guard. I think I added confusion when I said I wasn't worried about the extra size at sending the empty otel: { attributes: {} } in the seriliazed span. My bad.

I don't care strongly, but I think it would be nice to not send the empty otel: { attributes: {} } for every ListBuckets span.

Copy link
Member Author

@david-luna david-luna Apr 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree its better not to send empty properties in the span payload. For now Key property should always come with Bucket so putting back the attrs getter inside the if (bucket) block is enough.

Maybe in the future we want to send other params that are not paired with a Bucket so we will have to rethink the guard.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! restored the guard and modified the tests to check we're not sending empty otel

@trentm
Copy link
Member

trentm commented Apr 19, 2023

Regarding that one test failure (https://github.com/elastic/apm-agent-nodejs/actions/runs/4745685401/jobs/8428356662?pr=3274). I have been noticing a lot of spurious test failures with GH Actions recently. We may have to discuss some strategy to deal with those at some point. For now I have been (a) just re-running the failed jobs manually and (b) trying to note the particular spurious tests (in a local file for now) to see if there is a pattern to which ones tend to be flaky.

@trentm
Copy link
Member

trentm commented Apr 19, 2023

Oh, a thing I missed with my "approval": This should get a CHANGELOG.asciidoc entry as well.

@david-luna
Copy link
Member Author

Oh, a thing I missed with my "approval": This should get a CHANGELOG.asciidoc entry as well.

Added entry on changelog file :)

@david-luna david-luna marked this pull request as ready for review April 20, 2023 11:50
@trentm
Copy link
Member

trentm commented Apr 20, 2023

This is the 3rd attempt that has a failed test-vers (18, ...) job:

TAP version 13
# setup: npm ci (in /home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/instrumentation/azure-functions/fixtures/AJsAzureFnApp)
ok 1 "npm ci" succeeded (took 7.931s)
# azure functions
# setup
# mock APM apmServerUrl: http://localhost:45459
# setup: "func start" for AJsAzureFnApp fixture
# ["func start" stderr] /home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/instrumentation/azure-functions/fixtures/AJsAzureFnApp/node_modules/azure-functions-core-tools/bin/func: 2:
# ["func start" stderr] Syntax error: Unterminated quoted string
not ok 2 "func start" failed early: code=2
  ---
    operator: fail
    at: ChildProcess.onEarlyClose (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/instrumentation/azure-functions/azure-functions.test.js:484:9)
    stack: |-
      Error: "func start" failed early: code=2
          at Test.assert [as _assert] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:312:48)
          at Test.bound [as _assert] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:95:17)
          at Test.fail (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:406:7)
          at Test.bound [as fail] (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/node_modules/tape/lib/test.js:95:17)
          at ChildProcess.onEarlyClose (/home/runner/work/apm-agent-nodejs/apm-agent-nodejs/test/instrumentation/azure-functions/azure-functions.test.js:484:9)
          at ChildProcess.emit (node:events:513:28)
          at maybeClose (node:internal/child_process:1091:16)
          at ChildProcess._handle.onexit (node:internal/child_process:302:5)
  ...
# make requests
ok 3 there is no fnAppProc # SKIP
# check all APM events
ok 4 there is no fnAppProc # SKIP
# teardown

I cannot reproduce on macos. Still trying to repro otherwise.

@trentm
Copy link
Member

trentm commented Apr 20, 2023

Still trying to repro otherwise.

I am able to repro via .ci/scripts/test.sh -b release 18, so that's good. I'm trying on "main" and will open a separate issue if it reproduces there.

@trentm trentm mentioned this pull request Apr 20, 2023
Closed
@trentm
Copy link
Member

trentm commented Apr 20, 2023

The test failures for node 18 will get fixed with #3281

@trentm trentm merged commit 16d7c1d into elastic:main Apr 20, 2023
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
@david-luna @PeterEinberger
feat: add S3 Bucket and Key params in instrumentation (elastic#3274)
4ecf145 
Extract the parameters Bucket and Key from the instrumented requests to S3.
Parameters are added into OTel attributes of the span following the OTel semconv spec

Closes: elastic#3150
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trentm trentm trentm approved these changes

Assignees

@david-luna david-luna

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

Successfully merging this pull request may close these issues.

[META 759] Add S3 Bucket and Object Key to S3 instrumentation
2 participants
@david-luna @trentm