Skip to content

test: fix @aws-sdk/client-s3 TAV test failures #3312

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

Merged

Conversation

david-luna
Copy link
Member

@david-luna david-luna commented May 3, 2023

  • Fix for bad selection of docker compose file to run tests on module @aws-sdk/client-s3. Test script was selecting the default one instead of using one with localstack.
  • Bump the min supported client-s3 version to v3.15.0.
  • Also a small fix in intake event sorting for tests

Fixes: #3311

Checklist

@david-luna david-luna requested a review from trentm May 3, 2023 18:11
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label May 3, 2023
@david-luna
Copy link
Member Author

I've run the tests in local with the command

./.ci/scripts/test.sh -b "release" -t "@aws-sdk/client-s3" "20"

I'm still getting an error when testing v3.0.0 of the module though with a similar error message

docker-node_tests-1  | -- required packages ["@aws-sdk/[email protected]"]
docker-node_tests-1  | -- installing ["@aws-sdk/[email protected]"]
docker-node_tests-1  | -- running test "node test/instrumentation/modules/@aws-sdk/client-s3.test.js" with @aws-sdk/client-s3
docker-node_tests-1  | /app/test/instrumentation/modules/@aws-sdk/client-s3.test.js:105
docker-node_tests-1  |         const failingSpanId = spans[7].id // index of `getObjNonExistantObject`
docker-node_tests-1  |                                        ^
docker-node_tests-1  |
docker-node_tests-1  | TypeError: Cannot read properties of undefined (reading 'id')
docker-node_tests-1  |     at done (/app/test/instrumentation/modules/@aws-sdk/client-s3.test.js:105:40)
docker-node_tests-1  |     at ChildProcess.exithandler (node:child_process:427:5)
docker-node_tests-1  |     at ChildProcess.emit (node:events:511:28)
docker-node_tests-1  |     at maybeClose (node:internal/child_process:1098:16)
docker-node_tests-1  |     at ChildProcess._handle.onexit (node:internal/child_process:304:5)

@david-luna david-luna marked this pull request as ready for review May 3, 2023 18:45
@trentm
Copy link
Member

trentm commented May 3, 2023

I'm still getting an error when testing v3.0.0 of the module

Perhaps we don't need to go back that far for support. It was released in 2020:

  '3.0.0': '2020-12-15T23:03:11.641Z',

@trentm
Copy link
Member

trentm commented May 3, 2023

Our test starts failing at v3.14.0. I'll add a commit to make that our base supported version. This will broaden the scope of this PR to fix #3311

trentm added 2 commits May 3, 2023 12:51
Versions before that don't work with our current instrumentation.
v3.15.0 was released almost 2y ago, so this is fine.
Before this change the sorting would be broken on an 'error' intake
event. This could result in a spurious test failure if the intake
request happened to send the expected "error" event before the
"transaction" event was serialized and sent, resulting in a sorted
set like this:

    [
      { timestamp: 1683143107599773, id: 'ef6704', type: 'span' },
      { timestamp: 1683143107641831, id: '7282b2', type: 'span' },
      { timestamp: 1683143107661564, id: 'c8f2e5', type: 'span' },
      { timestamp: 1683143107687906, id: '826e4d', type: 'span' },
      { timestamp: 1683143107706615, id: 'be0dbf', type: 'span' },
      { timestamp: 1683143107830302, id: 'bcaa46', type: 'span' },
      { timestamp: 1683143107851506, id: '3edc44', type: 'span' },
      { timestamp: 1683143107874000, id: 'a03f5d', type: 'error' },
      { timestamp: 1683143107595038, id: 'a9a302', type: 'transaction' },  <-- not sorted by timestamp
      { timestamp: 1683143107874152, id: '02f35b', type: 'span' }
    ]
@trentm
Copy link
Member

trentm commented May 3, 2023

Some context on commit 396fd6c. I was testing locally with a manually started 'localstack' docker container. From an earlier test failure the test bucket had not been deleted. A subsequent test run resulted in the CreateBucket command not being run, so the expected spans were wrong. Because of the sorting bug the test error was this obtuse error:

not ok 3 got the transaction
  ---
    operator: ok
    expected: true
    actual:   undefined
    at: done (/Users/trentm/el/apm-agent-nodejs15/test/instrumentation/modules/@aws-sdk/client-s3.test.js:97:11)
    stack: |-
      Error: got the transaction
          at Test.assert [as _assert] (/Users/trentm/el/apm-agent-nodejs15/node_modules/tape/lib/test.js:312:48)
          at Test.bound [as _assert] (/Users/trentm/el/apm-agent-nodejs15/node_modules/tape/lib/test.js:95:17)
          at Test.assert (/Users/trentm/el/apm-agent-nodejs15/node_modules/tape/lib/test.js:431:7)
          at Test.bound [as ok] (/Users/trentm/el/apm-agent-nodejs15/node_modules/tape/lib/test.js:95:17)
          at done (/Users/trentm/el/apm-agent-nodejs15/test/instrumentation/modules/@aws-sdk/client-s3.test.js:97:11)
          at ChildProcess.exithandler (node:child_process:394:7)
          at ChildProcess.emit (node:events:513:28)
          at maybeClose (node:internal/child_process:1100:16)
          at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)
  ...
ok 4 span is valid (per apm-server intake schema)
ok 5 span is valid (per apm-server intake schema)
ok 6 span is valid (per apm-server intake schema)
ok 7 span is valid (per apm-server intake schema)
ok 8 span is valid (per apm-server intake schema)
ok 9 span is valid (per apm-server intake schema)
ok 10 span is valid (per apm-server intake schema)
/Users/trentm/el/apm-agent-nodejs15/test/instrumentation/modules/@aws-sdk/client-s3.test.js:108
        t.equal(spans.filter(s => s.trace_id === tx.trace_id).length,
                                                    ^

TypeError: Cannot read properties of undefined (reading 'trace_id')
    at /Users/trentm/el/apm-agent-nodejs15/test/instrumentation/modules/@aws-sdk/client-s3.test.js:108:53

After the sorting fix it was this clearer error:

not ok 17 createTheBucketIfNecessary produced expected span
  ---
    operator: deepEqual
    expected: |-
      { name: 'S3 CreateBucket elasticapmtest-bucket-3', type: 'storage', subtype: 's3', action: 'CreateBucket', context: { service: { target: { type: 's3', name: 'elasticapmtest-bucket-3' } }, destination: { address: 'localhost', port: 4566, cloud: { region: 'us-east-2' }, service: { type: '', name: '', resource: 'elasticapmtest-bucket-3' } }, http: { status_code: 200, response: { encoded_body_size: 177 } } }, otel: { attributes: { 'aws.s3.bucket': 'elasticapmtest-bucket-3' } }, outcome: 'success' }
    actual: |-
      { name: 'S3 HeadBucket elasticapmtest-bucket-3', type: 'storage', subtype: 's3', action: 'HeadBucket', context: { service: { target: { type: 's3', name: 'elasticapmtest-bucket-3' } }, destination: { address: 'localhost', port: 4566, service: { name: '', type: '', resource: 'elasticapmtest-bucket-3' }, cloud: { region: 'us-east-2' } }, http: { status_code: 200, response: { encoded_body_size: 0 } } }, outcome: 'success', otel: { attributes: { 'aws.s3.bucket': 'elasticapmtest-bucket-3' } } }
    at: done (/Users/trentm/el/apm-agent-nodejs15/test/instrumentation/modules/@aws-sdk/client-s3.test.js:147:11)
    stack: |-
      Error: createTheBucketIfNecessary produced expected span
          at Test.assert [as _assert] (/Users/trentm/el/apm-agent-nodejs15/node_modules/tape/lib/test.js:312:48)
          at Test.bound [as _assert] (/Users/trentm/el/apm-agent-nodejs15/node_modules/tape/lib/test.js:95:17)
          at Test.tapeDeepEqual (/Users/trentm/el/apm-agent-nodejs15/node_modules/tape/lib/test.js:553:7)
          at Test.bound [as deepEqual] (/Users/trentm/el/apm-agent-nodejs15/node_modules/tape/lib/test.js:95:17)
          at done (/Users/trentm/el/apm-agent-nodejs15/test/instrumentation/modules/@aws-sdk/client-s3.test.js:147:11)
          at ChildProcess.exithandler (node:child_process:394:7)
          at ChildProcess.emit (node:events:513:28)
          at maybeClose (node:internal/child_process:1100:16)
          at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)
  ...

@trentm trentm changed the title test: fix resolution of docker compose file for CI tests of client-s3 test: fix @aws-sdk/client-s3 TAV test failures May 3, 2023
@trentm trentm merged commit 64676bb into main May 3, 2023
@trentm trentm deleted the dluna/3311-ci-failing-aws-sdk-v3-bad-docker-compose-file branch May 3, 2023 20:11
v1v added a commit to v1v/apm-agent-nodejs that referenced this pull request May 8, 2023
…re/support-specific-modules

* 'main' of github.com:elastic/apm-agent-nodejs: (54 commits)
  chore: fix dev-utils/ci-tav-slow-jobs.sh (elastic#3319)
  test: reduce TAV test matrix for slowest jobs (elastic#3321)
  chore: sync package-lock so 'npm ci' can work (elastic#3318)
  docs: document `useElasticTraceparentHeader` config var (elastic#3316)
  chore, test: test driver improvements (elastic#3293)
  test: drop node 14 from RC tests now that it is EOL (elastic#3315)
  test: fix running fastify.test.js with node v8 (elastic#3317)
  feat: add @apollo/server@4 support (elastic#3203)
  chore: update nvm (elastic#3309)
  tests: stop testing 'express-graphql' instrumentation (elastic#3304)
  chore: fix bitrot.js dev util for recent changes (elastic#3308)
  test: restore testing of Azure Functions on node >=18.x (elastic#3307)
  fix: support Lambda instrumentation for `contextManager: 'patch'`; refactor Lambda tests (elastic#3305)
  test: fix fastify TAV test failures (elastic#3314)
  test: fix @aws-sdk/client-s3 TAV test failures (elastic#3312)
  feat: add instrumentation for aws-sdk S3 client (elastic#3287)
  feat(fastify): add captureBody support (elastic#2681)
  feat: mysql2@3 support (elastic#3301)
  chore(deps): bump @opentelemetry/exporter-prometheus from 0.37.0 to 0.38.0 in /test/opentelemetry-metrics/fixtures (elastic#3295)
  chore(deps-dev): bump fastify from 4.16.3 to 4.17.0 (elastic#3296)
  ...
trentm added a commit that referenced this pull request May 16, 2023
* fix resolution of docker compose file for CI tests of client-s3
* bump min supported @aws-sdk/client-s3 to v3.15.0
* Fix an issue in the tests sorting APM intake events to be tested.

Fixes: #3311 
Co-authored-by: Trent Mick <[email protected]>
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
* fix resolution of docker compose file for CI tests of client-s3
* bump min supported @aws-sdk/client-s3 to v3.15.0
* Fix an issue in the tests sorting APM intake events to be tested.

Fixes: elastic#3311 
Co-authored-by: Trent Mick <[email protected]>
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.

CI tests failing for @aws-sdk/client-s3 on main branch
2 participants