feat: AWS S3 instrumentation by trentm · Pull Request #2112 · elastic/apm-agent-nodejs

trentm · 2021-06-16T00:05:50Z

Fixes: #1954

Note that this PR will only include aws-sdk@2 instrumentation. A separate PR will address instrumentation of @awk-sdk/client-s3 (aka "v3" or the "Modular AWS SDK for JavaScript").

Checklist

ghost · 2021-06-16T00:09:49Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #2112 updated
Start Time: 2021-06-23T23:15:34.843+0000
Duration: 18 min 14 sec
Commit: ea9e8ed

Test stats 🧪

Test	Results
Failed	0
Passed	19040
Skipped	0
Total	19040

Trends 🧪

…is'll handle ESM imports

…10 because the package doesn't support older nodes

trentm · 2021-06-18T22:11:55Z

The state of instrumenting v3 of the JavaScript AWS SDK (https://github.com/aws/aws-sdk-js-v3/tree/main/clients/client-s3) is as follows. (See https://github.com/aws/aws-sdk-js-v3/tree/main/clients/client-s3#import for what I mean by the import styles.)

We can get it to automatically instrument if the CommonJS (a.k.a. ES5) require-style is used to import it.
We cannot automatically instrument if ESM (a.k.a. ES module, ES6+) import-style is used to import it. We cannot instrument it with known workarounds/hacks (e.g. this technique) because the package (@aws-sdk/client-s3) distributes separate source trees for CommonJS and ESM. (An import { ... } from "@aws-sdk/client-s3" and a const { ... } = require("@aws-sdk/client-s3") will load separate source files.)
As a possible documented workaround, we can provide a manual mechanism for users of import { ... } from "@aws-sdk/client-s3" to instrument their created client. It would look like this:

import apm from 'elastic-apm-node/start.js'
import { S3Client, HeadObjectCommand } from "@aws-sdk/client-s3";

const s3client = new S3Client({ region: 'us-west-1' })
apm.instrumentAwsSdkClient(s3client)   // <--- this is the only new line

// ...

I'm not sure if that is worthwhile.

… in GH actions

…ng node in tests Error: Command failed: /opt/hostedtoolcache/node/10.0.0/x64/bin/node fixtures/use-s3-callback-style.js openssl config failed: error:02001002:system library:fopen:No such file or directory

… run; better localstack healthcheck

…ion issue in CI

trentm · 2021-06-22T23:00:11Z

testing access points

S3 Access Points is a
separate AWS service that allows you to create a way to access a bucket
(or parts of a bucket) with policy separate from the bucket itself. It can
be useful for having multiple access policies for a single bucket. An access
point is tied to a specific S3 bucket and region. You then use the access
point ARN -- of the form
arn:aws:s3:$region:$accountId:accesspoint/$accesspointName -- instead of the
bucket name with the usual S3 client APIs, e.g.:

s3Client.getObject({
    Bucket: 'arn:aws:s3:us-west-1:123456789012:accesspoint/trentm-play-ap1',
    Key: 'aDir/aFile.txt'
}, function (err, data) {
    // ...
})

This comment documents my testing of the S3 instrumentation with Access Points.
It is not included in the test suite because, AFAIK, localstack doesn't support
access points.

First I created one to use:

% aws s3control create-access-point --account-id $accountId --bucket trentm-play-s3-bukkit2 --name trentm-play-ap1 --region us-west-1  # bucket is in us-west-1
% aws s3control list-access-points --account-id=$accountId --region=us-west-1
{
    "AccessPointList": [
        ...
        {
            "Name": "trentm-play-ap1",
            "NetworkOrigin": "Internet",
            "Bucket": "trentm-play-s3-bukkit2"
        }
    ]

In the v2 JavaScript AWS SDK, using an access point and the useArnRegion:true
config option for the client allows accessing a bucket in one region with a
client configured for a different region

With an access point the context.destination.address changes to a different
endpoint:

trentm-play-ap1-123456789012.s3-accesspoint.us-west-1.amazonaws.com
$accesspointName-$accountId.s3-accesspoint.$region.amazonaws.com

I used the following ARN in my dev script that exercises the S3 API a little bit.

const ARN1 = `arn:aws:s3:us-west-1:${accountId}:accesspoint/trentm-play-ap1`

A resulting span for a HeadObject call is:

    {
        "span": {
            "name": "S3 HeadObject accesspoint/trentm-play-ap1",
            "type": "storage",
            "subtype": "s3",
            "action": "HeadObject",
            "context": {
                "destination": {
                    "address": "trentm-play-ap1-123456789012.s3-accesspoint.us-west-1.amazonaws.com",
                    "port": 443,
                    "service": {
                        "name": "s3",
                        "type": "storage",
                        "resource": "accesspoint/trentm-play-ap1"
                    },
                    "cloud": {
                        "region": "us-west-1"
                    }
                }
            },
            "outcome": "success",
            // ...

trentm · 2021-06-22T23:21:52Z

.ci/scripts/docker-test.sh

 #   openssl config failed: error:25066067:DSO support routines:DLFCN_LOAD:could not load the shared library
 if [[ $major_node_version -gt 8 ]]; then
-  export NODE_OPTIONS="${NODE_OPTIONS:+${NODE_OPTIONS}} --openssl-config=./test/openssl-config-for-testing.cnf"
+  export NODE_OPTIONS="${NODE_OPTIONS:+${NODE_OPTIONS}} --openssl-config=$(pwd)/test/openssl-config-for-testing.cnf"


REVIEW NOTE: This was necessary to avoid this error message:

openssl config failed: error:02001002:system library:fopen:No such file or directory

when executing a subprocess to run node fixtures/use-s3.js as part of s3.test.js below... because that subprocess is run in a separate directory (in "test/instrumentation/modules/aws-sdk") rather than at the top-level. Using an absolute path fixes this -- which is the same thing that is done for this in test/test.js:

args.unshift('--require', path.join(__dirname, '_promise_rejection.js'))

trentm · 2021-06-22T23:23:14Z

.ci/scripts/test.sh

 fi

+ELASTIC_APM_ASYNC_HOOKS=${ELASTIC_APM_ASYNC_HOOKS:-true}
+


REVIEW NOTE: This ensures that ELASTIC_APM_ASYNC_HOOKS is either "true" or "false", and not "" or "false". When it is the empty string, things are fine, but the agent logs:

{"log.level":"warn","@timestamp":"2021-06-22T23:22:55.871Z","log":{"logger":"elastic-apm-node"},"ecs":{"version":"1.6.0"},"message":"unrecognized boolean value \"\" for \"asyncHooks\""} {"log.level":"warn","@timestamp":"2021-06-22T23:22:55.872Z","log":{"logger":"elastic-apm-node"},"ecs":{"version":"1.6.0"},"message":"unrecognized boolean value \"\" for \"asyncHooks\""}

which is just noise in test output.

trentm · 2021-06-22T23:43:34Z

@astorm I haven't yet added a changelog entry, but this is ready for review. Our AWS instrumentation might benefit from a separate documentation page, but I think that could be done in a separate issue.

trentm · 2021-06-23T16:34:02Z

That "Integration Tests failure" above (https://apm-ci.elastic.co/job/apm-agent-nodejs/job/apm-agent-nodejs-mbp/job/PR-2112/) has this in the log:

[2021-06-23T00:26:18.539Z] ERROR: for apm-server  Container "83e98b03806a" is unhealthy.
[2021-06-23T00:26:18.539Z] Encountered errors while bringing up the project.
[2021-06-23T00:26:18.539Z] Starting/Building stack services..
[2021-06-23T00:26:18.539Z] 
[2021-06-23T00:26:18.539Z] Traceback (most recent call last):
[2021-06-23T00:26:18.539Z]   File "scripts/compose.py", line 19, in <module>
[2021-06-23T00:26:18.539Z]     main()
[2021-06-23T00:26:18.539Z]   File "scripts/compose.py", line 15, in main
[2021-06-23T00:26:18.539Z]     setup()
[2021-06-23T00:26:18.539Z]   File "/var/lib/jenkins/workspace/ration-tests-selector-mbp_master/src/github.com/elastic/apm-integration-testing/scripts/modules/cli.py", line 204, in __call__
[2021-06-23T00:26:18.539Z]     self.args.func()
[2021-06-23T00:26:18.539Z]   File "/var/lib/jenkins/workspace/ration-tests-selector-mbp_master/src/github.com/elastic/apm-integration-testing/scripts/modules/cli.py", line 586, in start_handler
[2021-06-23T00:26:18.539Z]     self.build_start_handler("start")
[2021-06-23T00:26:18.540Z]   File "/var/lib/jenkins/workspace/ration-tests-selector-mbp_master/src/github.com/elastic/apm-integration-testing/scripts/modules/cli.py", line 732, in build_start_handler
[2021-06-23T00:26:18.540Z]     self.run_docker_compose_process(docker_compose_cmd + up_params)
[2021-06-23T00:26:18.540Z]   File "/var/lib/jenkins/workspace/ration-tests-selector-mbp_master/src/github.com/elastic/apm-integration-testing/scripts/modules/cli.py", line 472, in run_docker_compose_process
[2021-06-23T00:26:18.540Z]     subprocess.check_call(docker_compose_cmd)
[2021-06-23T00:26:18.540Z]   File "/usr/lib/python3.6/subprocess.py", line 311, in check_call
[2021-06-23T00:26:18.540Z]     raise CalledProcessError(retcode, cmd)
[2021-06-23T00:26:18.540Z] subprocess.CalledProcessError: Command '['docker-compose', '-f', '/var/lib/jenkins/workspace/ration-tests-selector-mbp_master/src/github.com/elastic/apm-integration-testing/docker-compose.yml', '--no-ansi', '--log-level', 'ERROR', 'up', '-d', '--quiet-pull']' returned non-zero exit status 1.

I don't know whta that is about. I'll try to run it again -> https://apm-ci.elastic.co/job/apm-integration-tests-selector-mbp/job/master/18016/

astorm

Overall looks good.

Just to say it out loud I do have mixed feeling about incorporating another dependency (localstack) directly into our CI ... but I think I'm slightly in favor of it if it can get us out of the business of mocking requests (especially with the specre of a new SDK with different requests to mock out)

Review is just some due diligence style questions and some nits. Trending towards approval once those questions are answered. 👍

test/script/local-deps-start.sh

test/instrumentation/modules/aws-sdk/s3.test.js

lib/instrumentation/modules/aws-sdk/s3.js

package.json

lib/instrumentation/modules/aws-sdk.js

lib/instrumentation/modules/aws-sdk/s3.js

astorm

👍

Note that this only includes 'aws-sdk@2' instrumentation. Instrumentation of '@awk-sdk/client-s3' (aka "v3" or the "Modular AWS SDK for JavaScript") will be done in a separate change. This adds localstack to the test suite for testing the S3 API. Fixes: elastic#1954

trentm self-assigned this Jun 16, 2021

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jun 16, 2021

feat: AWS S3 instrumentation

ccc7fd7

trentm force-pushed the trentm/instrument-s3 branch from 3ad0cb1 to ccc7fd7 Compare June 16, 2021 00:08

trentm added 4 commits June 16, 2021 11:58

drop setting span.context.db.* for S3 spans for now pending discussion

a48477c

aws-sdk@2 S3 instrumentation should be good with this commit

9410ae5

first partial impl of @awk-sdk/client-s3 instrumentation; doubtful th…

2b48d1a

…is'll handle ESM imports

avoid 'disableInstrumentations' test of @aws-sdk/client-s3 on node <v…

cec0575

…10 because the package doesn't support older nodes

trentm added 14 commits June 18, 2021 16:39

v3 SDK: destination context, error capture, outcome

0cf170a

clean up TODOs in the s3 instrumentations

26310ba

first crack at testing S3 aws-sdk instrumentation using localstack

0c9d3be

github actions need to setup localstack as well; drop node 15 testing…

642b58d

… in GH actions

correct GH action yaml syntax

5305b47

fix the following "openssl ... No such file or directory" when exec'i…

bdb89df

…ng node in tests Error: Command failed: /opt/hostedtoolcache/node/10.0.0/x64/bin/node fixtures/use-s3-callback-style.js openssl config failed: error:02001002:system library:fopen:No such file or directory

debugging CI test failures

7006d39

limit tests run for debugging failures; fix AWS auth envvars for test…

fe0b0a1

… run; better localstack healthcheck

not helping standard

d42619b

log used endpoint for debugging; add .promise()-style case

0adb071

fixes; still debugging CI

1eb42da

's3ForcePathStyle: true' to fix the bucketName.localstack DNS resolut…

585206a

…ion issue in CI

windows test fix

e8067e4

cleaning up tests

b157b25

trentm added 2 commits June 22, 2021 16:04

restore full node ver testing; modulo dropping node v15

e89a021

restore runing all the test files

d1239ab

trentm mentioned this pull request Jun 22, 2021

feat: instrument @aws-sdk/client-s3 #2124

Closed

5 tasks

trentm commented Jun 22, 2021

View reviewed changes

drop v3 SDK instrumentation, it has moved to a separate PR

44c860d

Merge branch 'master' into trentm/instrument-s3

31da8a1

trentm marked this pull request as ready for review June 22, 2021 23:42

trentm requested a review from astorm June 22, 2021 23:42

method S3 is supported tech

a1892d5

trentm mentioned this pull request Jun 22, 2021

drop HTTP spans under S3 spans #2125

Closed

changelog entry

5455623

Merge branch 'master' into trentm/instrument-s3

79cfb27

astorm reviewed Jun 23, 2021

View reviewed changes

updates from review feedback

ea9e8ed

trentm requested a review from astorm June 23, 2021 23:16

astorm approved these changes Jun 24, 2021

View reviewed changes

trentm merged commit 9ece910 into master Jun 24, 2021

trentm deleted the trentm/instrument-s3 branch June 24, 2021 23:19

This was referenced Jun 28, 2021

aws-sdk TAV test failure with localstack docker volume setup #2129

Closed

tests: run S3 tests with each aws-sdk TAV version test #2136

Merged

		fi

		ELASTIC_APM_ASYNC_HOOKS=${ELASTIC_APM_ASYNC_HOOKS:-true}

Conversation

trentm commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ghost commented Jun 16, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

Uh oh!

trentm commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trentm commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

testing access points

Uh oh!

trentm Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

trentm Jun 22, 2021

Choose a reason for hiding this comment

Uh oh!

trentm commented Jun 22, 2021

Uh oh!

trentm commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astorm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astorm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

trentm commented Jun 16, 2021 •

edited

Loading

ghost commented Jun 16, 2021 •

edited by ghost

Loading

trentm commented Jun 18, 2021 •

edited

Loading

trentm commented Jun 22, 2021 •

edited

Loading

trentm commented Jun 23, 2021 •

edited

Loading