Skip to content

CI tests failing for @aws-sdk/client-s3 on main branch #3311

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

Closed
david-luna opened this issue May 3, 2023 · 3 comments · Fixed by #3312
Closed

CI tests failing for @aws-sdk/client-s3 on main branch #3311

david-luna opened this issue May 3, 2023 · 3 comments · Fixed by #3312
Assignees
Labels
agent-nodejs Make available for APM Agents project planning. build-failures Build failures in the CI.

Comments

@david-luna
Copy link
Member

This problem is originated by the merge of PR #3287 . The CI tests on PR worked fine but now they're failing on main branch. the logs show we are using the default docker compose file for node.

.ci/scripts/test.sh:191: case ${TAV_MODULE} in
.ci/scripts/test.sh:221: DOCKER_COMPOSE_FILE=docker-compose-node-test.yml
.ci/scripts/test.sh:229: set +e
.ci/scripts/test.sh:230: NVM_NODEJS_ORG_MIRROR=
.ci/scripts/test.sh:230: ELASTIC_APM_CONTEXT_MANAGER=
.ci/scripts/test.sh:230: NODE_VERSION=20
.ci/scripts/test.sh:230: NODE_FULL_VERSION=
.ci/scripts/test.sh:230: TAV_MODULE=@aws-sdk/client-s3
..ci/scripts/test.sh:230: id -u
..ci/scripts/test.sh:230: id -g
.ci/scripts/test.sh:230: USER_ID=1001:[12](https://github.com/elastic/apm-agent-nodejs/actions/runs/4871077219/jobs/8687612181#step:3:13)3
.ci/scripts/test.sh:230: docker-compose --no-ansi --log-level ERROR -f .ci/docker/docker-compose-node-test.yml build
.ci/scripts/test.sh:242: '[' 0 -gt 0 ']'
.ci/scripts/test.sh:249: set -e
.ci/scripts/test.sh:250: NVM_NODEJS_ORG_MIRROR=
.ci/scripts/test.sh:250: ELASTIC_APM_CONTEXT_MANAGER=
.ci/scripts/test.sh:250: NODE_VERSION=[20](https://github.com/elastic/apm-agent-nodejs/actions/runs/4871077219/jobs/8687612181#step:3:21)
.ci/scripts/test.sh:250: NODE_FULL_VERSION=
.ci/scripts/test.sh:250: TAV_MODULE=@aws-sdk/client-s3
..ci/scripts/test.sh:250: id -u
..ci/scripts/test.sh:250: id -g
.ci/scripts/test.sh:250: USER_ID=1001:1[23](https://github.com/elastic/apm-agent-nodejs/actions/runs/4871077219/jobs/8687612181#step:3:24)
.ci/scripts/test.sh:[25](https://github.com/elastic/apm-agent-nodejs/actions/runs/4871077219/jobs/8687612181#step:3:26)0: docker-compose --no-ansi --log-level ERROR -f .ci/docker/docker-compose-node-test.yml up --exit-code-from node_tests --remove-orphans --abort-on-container-exit node_tests
--no-ansi option is deprecated and will be removed in future versions. Use `--ansi never` instead.
Creating docker_node_tests_1 ... 
Creating docker_node_tests_1 ... done

We should use the same compose file we're using to test earlier versions of aws-sdk which os located at .ci/docker/docker-compose-localstack.yml. I think we should add a new entry in this switch statement

@david-luna david-luna added agent-nodejs Make available for APM Agents project planning. build-failures Build failures in the CI. labels May 3, 2023
@david-luna david-luna self-assigned this May 3, 2023
@trentm
Copy link
Member

trentm commented May 3, 2023

Should be able to test locally with:

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

I think we should add a new entry in this switch statement

That sounds right.

@trentm
Copy link
Member

trentm commented May 3, 2023

From the https://github.com/elastic/apm-agent-nodejs/actions/runs/4871077219/jobs/8687612181 failure log:

...
 node_tests_1  | # use-client-s3.js stdout:
node_tests_1  | # {"log.level":"info","@timestamp":"2023-05-03T10:56:45.579Z","log":{"logger":"elastic-apm-node"},"ecs":{"version":"1.6.0"},"message":"Sending error to Elastic APM: {\"id\":\"7b33a6cd9ed12b01816dde290c689f3a\"}"}
node_tests_1  | # use-client-s3.js stderr:

The reason the actual exception isn't printed is because logUncaughtExceptions is false by default. I want that to be true by default in the next major rev. For now it might be helpful for debugging to make this change:

diff --git a/test/instrumentation/modules/@aws-sdk/fixtures/use-client-s3.js b/test/instrumentation/modules/@aws-sdk/fixtures/use-client-s3.js
index 1d93f3df..bd90ea79 100644
--- a/test/instrumentation/modules/@aws-sdk/fixtures/use-client-s3.js
+++ b/test/instrumentation/modules/@aws-sdk/fixtures/use-client-s3.js
@@ -46,6 +46,7 @@ const apm = require('../../../../..').start({
   centralConfig: false,
   metricsInterval: 0,
   cloudProvider: 'none',
+  logUncaughtExceptions: true,
   stackTraceLimit: 4, // get it smaller for reviewing output
   logLevel: 'info'
 })

@trentm
Copy link
Member

trentm commented May 3, 2023

The reason the actual exception isn't printed is because logUncaughtExceptions is false by default.

Nope, I'm wrong. This isn't an uncaught exception. By dumping the intake events sent to the mock APM server we can see the reported error:

node_tests_1  |   {
node_tests_1  |     error: {
node_tests_1  |       id: 'cbe881da3ab362aead3c28d96cb7d31d',
node_tests_1  |       timestamp: 1683140360924000,
node_tests_1  |       parent_id: '60348bfd9f327b0a',
node_tests_1  |       trace_id: '5772f16f4649bf84297dff776e77dd35',
node_tests_1  |       transaction_id: '8f418de82384a130',
node_tests_1  |       transaction: { name: 'manual', type: 'custom', sampled: true },
node_tests_1  |       context: { user: {}, tags: {}, custom: {} },
node_tests_1  |       exception: {
node_tests_1  |         message: 'getaddrinfo ENOTFOUND localstack',
node_tests_1  |         type: 'Error',
node_tests_1  |         handled: true,
node_tests_1  |         code: 'ENOTFOUND',
node_tests_1  |         attributes: {
node_tests_1  |           errno: -3008,
node_tests_1  |           syscall: 'getaddrinfo',
node_tests_1  |           hostname: 'localstack'
node_tests_1  |         },
node_tests_1  |         stacktrace: [
node_tests_1  |           {
node_tests_1  |             filename: 'node:dns',
node_tests_1  |             lineno: 118,
node_tests_1  |             function: 'onlookupall',
node_tests_1  |             library_frame: true,
node_tests_1  |             abs_path: 'node:dns'
node_tests_1  |           },
node_tests_1  |           {
node_tests_1  |             filename: 'node:internal/async_hooks',
node_tests_1  |             lineno: 130,
node_tests_1  |             function: 'callbackTrampoline',
node_tests_1  |             library_frame: true,
node_tests_1  |             abs_path: 'node:internal/async_hooks'
node_tests_1  |           }
node_tests_1  |         ]
node_tests_1  |       },
node_tests_1  |       culprit: 'onlookupall (node:dns)'
node_tests_1  |     }
node_tests_1  |   }

trentm added a commit that referenced this issue May 3, 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]>
trentm added a commit that referenced this issue 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 issue 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. build-failures Build failures in the CI.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants