From ccc7fd7879deb5e7d3abca6880db88ff6e6dbe2e Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 15 Jun 2021 17:04:53 -0700 Subject: [PATCH 01/25] feat: AWS S3 instrumentation --- lib/instrumentation/modules/aws-sdk.js | 12 ++- lib/instrumentation/modules/aws-sdk/s3.js | 116 +++++++++++++++++++++ lib/instrumentation/modules/aws-sdk/sqs.js | 1 + 3 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 lib/instrumentation/modules/aws-sdk/s3.js diff --git a/lib/instrumentation/modules/aws-sdk.js b/lib/instrumentation/modules/aws-sdk.js index 67c133c1fe..f6baf8bd31 100644 --- a/lib/instrumentation/modules/aws-sdk.js +++ b/lib/instrumentation/modules/aws-sdk.js @@ -1,19 +1,27 @@ 'use strict' const semver = require('semver') const shimmer = require('../shimmer') +const { instrumentationS3 } = require('./aws-sdk/s3') const { instrumentationSqs } = require('./aws-sdk/sqs') +const instrumentorFromSvcId = { + s3: instrumentationS3, + sqs: instrumentationSqs +} + // Called in place of AWS.Request.send and AWS.Request.promise // // Determines which amazon service an API request is for // and then passes call on to an appropriate instrumentation // function. function instrumentOperation (orig, origArguments, request, AWS, agent, { version, enabled }) { - if (request.service.serviceIdentifier === 'sqs') { - return instrumentationSqs(orig, origArguments, request, AWS, agent, { version, enabled }) + const instrumentor = instrumentorFromSvcId[request.service.serviceIdentifier] + if (instrumentor) { + return instrumentor(orig, origArguments, request, AWS, agent, { version, enabled }) } // if we're still here, then we still need to call the original method + console.warn('XXX serviceIdentifier', request.service.serviceIdentifier) return orig.apply(request, origArguments) } diff --git a/lib/instrumentation/modules/aws-sdk/s3.js b/lib/instrumentation/modules/aws-sdk/s3.js new file mode 100644 index 0000000000..2287062f45 --- /dev/null +++ b/lib/instrumentation/modules/aws-sdk/s3.js @@ -0,0 +1,116 @@ +'use strict' + +// Instrument AWS S3 operations via the 'aws-sdk' package. + +const constants = require('../../../constants') + +const TYPE = 'storage' +const SUBTYPE = 's3' + +// This regex adapted from "OutpostArn" pattern at +// https://docs.aws.amazon.com/outposts/latest/APIReference/API_Outpost.html +const ARN_REGEX = /^arn:aws([a-z-]+)?:(s3|s3-outposts):[a-z\d-]+:\d{12}:(.*?)$/ + +// Instrument an awk-sdk@2.x operation (i.e. a AWS.Request.send or +// AWS.Request.promise). +// +// @param {AWS.Request} request https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Request.html +function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, enabled }) { + // console.warn('XXX endpoint: ', request.service.endpoint) + // console.warn('XXX service.config before: ', request.service.config) + + // XXX action is 'listBuckets' from the SDK. Our spec is 'ListBuckets'. Is a + // simple string.capitalize sufficient? TODO: Find a aws-sdk-js-v3 ref for that. + const action = request.operation[0].toUpperCase() + request.operation.slice(1) + let name = 'S3 ' + action + let resource = null // The bucket name or normalized Access Point/Outpost ARN. + if (request.params && request.params.Bucket) { + resource = request.params.Bucket + if (resource.startsWith('arn:')) { + // This is an Access Point or Outpost ARN, e.g.: + // - arn:aws:s3:region:account-id:accesspoint/resource + // - arn:aws:s3-outposts:::outpost//bucket/ + // - arn:aws:s3-outposts:::outpost//accesspoint/ + const match = ARN_REGEX.exec(resource) + if (match) { + resource = match[3] + } + } + name += ' ' + resource + } + + const span = agent.startSpan(name, TYPE, SUBTYPE, action) + if (span) { + // Determining the bucket region is not always possible. + // - `request.service.config.region` can be "us-west-2" and talk to a bucket + // in "us-west-1" + // - `request.service.endpoint` can be to a regional endpoint, e.g. + // "https://s3.us-east-2.amazonaws.com", to access a bucket hosted in a + // different region. + // - The `x-amz-bucket-region` response header is set for HeadBucket calls. + // - A separate API call to GetBucketLocation will return the + // "LocationConstraint" specified at CreateBucket. API docs + // (https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#getBucketLocation-property) + // state "Buckets in Region us-east-1 have a LocationConstraint of null." + // though the empty string has been observed. + // - If there is a `response.error` set, it sometimes has a 'region' prop. + span.setDbContext({ + type: 's3' + // instance: region // XXX see above + }) + + // Destination context. + const endpoint = request.service.endpoint + const destContext = { + address: endpoint.hostname, + port: endpoint.port, + service: { + name: 's3', + type: 'storage' + } + } + if (resource) { + destContext.service.resource = resource + } + // TODO: destination.cloud.region is pending https://github.com/elastic/ecs/issues/1282 + span.setDestinationContext(destContext) + + request.on('complete', function (response) { + // console.warn('XXX response: ', response) + // console.warn('XXX response info: ', + // 'retryCount', response.retryCount, + // 'redirectCount', response.redirectCount, + // 'statusCode', response.httpResponse.statusCode, + // 'headers', response.httpResponse.headers + // ) + if (response && response.error) { + console.warn('XXX response.error ', response.error) + // XXX is skipOutcome necessary for S3 error responses? + const errOpts = { + skipOutcome: true + } + agent.captureError(response.error, errOpts) + span._setOutcomeFromErrorCapture(constants.OUTCOME_FAILURE) + } + + // Workaround a bug in the agent's handling of `span.sync`. + // + // The bug: Currently this span.sync is not set `false` because there is + // an HTTP span created (for this S3 request) in the same async op. That + // HTTP span becomes the "active span" for this async op, and *it* gets + // marked as sync=false in `before()` in async-hooks.js. + // TODO: move this to a separate issue. + span.sync = false + + span.end() + }) + } + + const origResult = orig.apply(request, origArguments) + + return origResult +} + +module.exports = { + instrumentationS3 +} diff --git a/lib/instrumentation/modules/aws-sdk/sqs.js b/lib/instrumentation/modules/aws-sdk/sqs.js index 92201b2a85..0c05e15936 100644 --- a/lib/instrumentation/modules/aws-sdk/sqs.js +++ b/lib/instrumentation/modules/aws-sdk/sqs.js @@ -152,6 +152,7 @@ function instrumentationSqs (orig, origArguments, request, AWS, agent, { version const action = getActionFromRequest(request) const name = getSpanNameFromRequest(request) const span = agent.startSpan(name, type, subtype, action) + // XXX span can be *null* here if no trans, right?! span.setDestinationContext(getMessageDestinationContextFromRequest(request)) span.setMessageContext(getMessageContextFromRequest(request)) From a48477c2bc24c5cb66e6792ffa0841baa0646828 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Wed, 16 Jun 2021 11:58:53 -0700 Subject: [PATCH 02/25] drop setting span.context.db.* for S3 spans for now pending discussion --- lib/instrumentation/modules/aws-sdk/s3.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/instrumentation/modules/aws-sdk/s3.js b/lib/instrumentation/modules/aws-sdk/s3.js index 2287062f45..ff5aa4c5b5 100644 --- a/lib/instrumentation/modules/aws-sdk/s3.js +++ b/lib/instrumentation/modules/aws-sdk/s3.js @@ -54,10 +54,13 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, // state "Buckets in Region us-east-1 have a LocationConstraint of null." // though the empty string has been observed. // - If there is a `response.error` set, it sometimes has a 'region' prop. - span.setDbContext({ - type: 's3' - // instance: region // XXX see above - }) + // const region = null // XXX + + // XXX context.db is pending discussion + // span.setDbContext({ + // type: 's3' + // // instance: region // XXX see above + // }) // Destination context. const endpoint = request.service.endpoint From 9410ae5f6296b13325036d4ff1eb4a10943c2652 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Wed, 16 Jun 2021 16:54:46 -0700 Subject: [PATCH 03/25] aws-sdk@2 S3 instrumentation should be good with this commit --- lib/agent.js | 1 + lib/instrumentation/modules/aws-sdk.js | 1 - lib/instrumentation/modules/aws-sdk/s3.js | 115 ++++++++++----------- lib/instrumentation/modules/aws-sdk/sqs.js | 1 - 4 files changed, 55 insertions(+), 63 deletions(-) diff --git a/lib/agent.js b/lib/agent.js index 1b7b73507b..0877320d5c 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -321,6 +321,7 @@ const EMPTY_OPTS = {} // // Usage: // captureError(err, opts, cb) +// captureError(err, opts) // captureError(err, cb) // // where: diff --git a/lib/instrumentation/modules/aws-sdk.js b/lib/instrumentation/modules/aws-sdk.js index f6baf8bd31..f5af25b22d 100644 --- a/lib/instrumentation/modules/aws-sdk.js +++ b/lib/instrumentation/modules/aws-sdk.js @@ -21,7 +21,6 @@ function instrumentOperation (orig, origArguments, request, AWS, agent, { versio } // if we're still here, then we still need to call the original method - console.warn('XXX serviceIdentifier', request.service.serviceIdentifier) return orig.apply(request, origArguments) } diff --git a/lib/instrumentation/modules/aws-sdk/s3.js b/lib/instrumentation/modules/aws-sdk/s3.js index ff5aa4c5b5..571b2ed6af 100644 --- a/lib/instrumentation/modules/aws-sdk/s3.js +++ b/lib/instrumentation/modules/aws-sdk/s3.js @@ -2,8 +2,6 @@ // Instrument AWS S3 operations via the 'aws-sdk' package. -const constants = require('../../../constants') - const TYPE = 'storage' const SUBTYPE = 's3' @@ -16,13 +14,11 @@ const ARN_REGEX = /^arn:aws([a-z-]+)?:(s3|s3-outposts):[a-z\d-]+:\d{12}:(.*?)$/ // // @param {AWS.Request} request https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Request.html function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, enabled }) { - // console.warn('XXX endpoint: ', request.service.endpoint) - // console.warn('XXX service.config before: ', request.service.config) - - // XXX action is 'listBuckets' from the SDK. Our spec is 'ListBuckets'. Is a - // simple string.capitalize sufficient? TODO: Find a aws-sdk-js-v3 ref for that. - const action = request.operation[0].toUpperCase() + request.operation.slice(1) - let name = 'S3 ' + action + // Get the PascalCase operation name from `request.operation` by undoing to + // `lowerFirst()` from + // https://github.com/aws/aws-sdk-js/blob/c0c44b8a4e607aae521686898f39a3e359f727e4/lib/model/api.js#L63-L65 + const opName = request.operation[0].toUpperCase() + request.operation.slice(1) + let name = 'S3 ' + opName let resource = null // The bucket name or normalized Access Point/Outpost ARN. if (request.params && request.params.Bucket) { resource = request.params.Bucket @@ -39,61 +35,60 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, name += ' ' + resource } - const span = agent.startSpan(name, TYPE, SUBTYPE, action) + const span = agent.startSpan(name, TYPE, SUBTYPE, opName) if (span) { - // Determining the bucket region is not always possible. - // - `request.service.config.region` can be "us-west-2" and talk to a bucket - // in "us-west-1" - // - `request.service.endpoint` can be to a regional endpoint, e.g. - // "https://s3.us-east-2.amazonaws.com", to access a bucket hosted in a - // different region. - // - The `x-amz-bucket-region` response header is set for HeadBucket calls. - // - A separate API call to GetBucketLocation will return the - // "LocationConstraint" specified at CreateBucket. API docs - // (https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#getBucketLocation-property) - // state "Buckets in Region us-east-1 have a LocationConstraint of null." - // though the empty string has been observed. - // - If there is a `response.error` set, it sometimes has a 'region' prop. - // const region = null // XXX + request.on('complete', function onComplete (response) { + // `response` is an AWS.Response + // https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Response.html - // XXX context.db is pending discussion - // span.setDbContext({ - // type: 's3' - // // instance: region // XXX see above - // }) + // Determining the bucket's region. + // `request.httpRequest.region` isn't documented, but the aws-sdk@2 + // lib/services/s3.js will often set it to the bucket's determined region. + // This can be asynchronously determined -- e.g. if it differs from the + // configured service endpoint region -- so this won't be set until + // 'complete'. + const region = request.httpRequest.region - // Destination context. - const endpoint = request.service.endpoint - const destContext = { - address: endpoint.hostname, - port: endpoint.port, - service: { - name: 's3', - type: 'storage' + // Destination context. + // '.httpRequest.endpoint' might differ from '.service.endpoint' if + // the bucket is in a different region. + const endpoint = request.httpRequest.endpoint + const destContext = { + address: endpoint.hostname, + port: endpoint.port, + service: { + name: SUBTYPE, + type: TYPE + } } - } - if (resource) { - destContext.service.resource = resource - } - // TODO: destination.cloud.region is pending https://github.com/elastic/ecs/issues/1282 - span.setDestinationContext(destContext) + if (resource) { + destContext.service.resource = resource + } + if (region) { + destContext.cloud = { region } + } + span.setDestinationContext(destContext) + + // XXX I think the following onComplete handling could be shared for all AWS instrumentations - request.on('complete', function (response) { - // console.warn('XXX response: ', response) - // console.warn('XXX response info: ', - // 'retryCount', response.retryCount, - // 'redirectCount', response.redirectCount, - // 'statusCode', response.httpResponse.statusCode, - // 'headers', response.httpResponse.headers - // ) - if (response && response.error) { - console.warn('XXX response.error ', response.error) - // XXX is skipOutcome necessary for S3 error responses? - const errOpts = { - skipOutcome: true + if (response) { // insanity guard + // Follow the spec for HTTP client span outcome. + // https://github.com/elastic/apm/blob/master/specs/agents/tracing-instrumentation-http.md#outcome + // + // For example, a S3 GetObject conditional request (e.g. using the + // IfNoneMatch param) will respond with response.error=NotModifed and + // statusCode=304. This is a *successful* outcome. + const statusCode = response.httpResponse.statusCode + span._setOutcomeFromHttpStatusCode(statusCode) + + if (response.error && statusCode && statusCode >= 400) { + // `skipOutcome` because (a) we have set it manually per the + // statusCode, and (b) `captureError` uses `this.currentSpan` which + // currently *gets the wrong span* -- it gets the HTTP span created + // in the same async op for the underlying HTTP request to the AWS + // endpoint. + agent.captureError(response.error, { skipOutcome: true }) } - agent.captureError(response.error, errOpts) - span._setOutcomeFromErrorCapture(constants.OUTCOME_FAILURE) } // Workaround a bug in the agent's handling of `span.sync`. @@ -109,9 +104,7 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, }) } - const origResult = orig.apply(request, origArguments) - - return origResult + return orig.apply(request, origArguments) } module.exports = { diff --git a/lib/instrumentation/modules/aws-sdk/sqs.js b/lib/instrumentation/modules/aws-sdk/sqs.js index 0c05e15936..92201b2a85 100644 --- a/lib/instrumentation/modules/aws-sdk/sqs.js +++ b/lib/instrumentation/modules/aws-sdk/sqs.js @@ -152,7 +152,6 @@ function instrumentationSqs (orig, origArguments, request, AWS, agent, { version const action = getActionFromRequest(request) const name = getSpanNameFromRequest(request) const span = agent.startSpan(name, type, subtype, action) - // XXX span can be *null* here if no trans, right?! span.setDestinationContext(getMessageDestinationContextFromRequest(request)) span.setMessageContext(getMessageContextFromRequest(request)) From 2b48d1ae5fb166493af6d28fc00da9fbb3a69105 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Thu, 17 Jun 2021 17:52:59 -0700 Subject: [PATCH 04/25] first partial impl of @awk-sdk/client-s3 instrumentation; doubtful this'll handle ESM imports --- lib/instrumentation/index.js | 1 + .../modules/@aws-sdk/client-s3.js | 203 ++++++++++++++++++ package.json | 1 + 3 files changed, 205 insertions(+) create mode 100644 lib/instrumentation/modules/@aws-sdk/client-s3.js diff --git a/lib/instrumentation/index.js b/lib/instrumentation/index.js index fa33f55c7b..55dbdef997 100644 --- a/lib/instrumentation/index.js +++ b/lib/instrumentation/index.js @@ -11,6 +11,7 @@ var shimmer = require('./shimmer') var Transaction = require('./transaction') var MODULES = [ + '@aws-sdk/client-s3', '@elastic/elasticsearch', 'apollo-server-core', 'aws-sdk', diff --git a/lib/instrumentation/modules/@aws-sdk/client-s3.js b/lib/instrumentation/modules/@aws-sdk/client-s3.js new file mode 100644 index 0000000000..e72437a83c --- /dev/null +++ b/lib/instrumentation/modules/@aws-sdk/client-s3.js @@ -0,0 +1,203 @@ +'use strict' + +// Instrument the @aws-sdk/client-s3 package. +// https://github.com/aws/aws-sdk-js-v3/tree/main/clients/client-s3 +// +// XXX doc technique used + +const TYPE = 'storage' +const SUBTYPE = 's3' + +// For a HeadObject API call, `context.commandName === 'HeadObjectCommand'`. +const COMMAND_NAME_RE = /^(\w+)Command$/ +function opNameFromCommandName (commandName) { + const match = COMMAND_NAME_RE.exec(commandName) + if (match) { + return match[1] + } else { + return '' + } +} + +// This regex adapted from "OutpostArn" pattern at +// https://docs.aws.amazon.com/outposts/latest/APIReference/API_Outpost.html +const ARN_REGEX = /^arn:aws([a-z-]+)?:(s3|s3-outposts):[a-z\d-]+:\d{12}:(.*?)$/ + +function resourceFromParams (params) { + let resource = null // The bucket name or normalized Access Point/Outpost ARN. + if (params && params.Bucket) { + resource = params.Bucket + if (resource.startsWith('arn:')) { + // This is an Access Point or Outpost ARN, e.g.: + // - arn:aws:s3:region:account-id:accesspoint/resource + // - arn:aws:s3-outposts:::outpost//bucket/ + // - arn:aws:s3-outposts:::outpost//accesspoint/ + const match = ARN_REGEX.exec(resource) + if (match) { + resource = match[3] + } + } + } + return resource +} + +module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabled }) { + if (!enabled) { + return mod + } + if (!mod.S3Client) { + agent.logger.debug('@aws-sdk/client-s3@%s is not supported (no `S3Client`) - aborting...', version) + return mod + } + + class ApmS3Client extends mod.S3Client { + constructor (...args) { + super(...args) + + const client = this + + this.middlewareStack.add( + (next, context) => async (args) => { + console.warn('XXX context at finalizeRequest time', context) + console.warn('XXX args at finalizeRequest time', args) + // XXX set destination context here, + // what was `hostname: 's3.us-west-1.amazonaws.com'` at build + // time is now `hostname: 'trentm-play-s3-bukkit2.s3.us-west-1.amazonaws.com',` + return await next(args) + }, + { + step: 'finalizeRequest' + } + ) + // this.middlewareStack.add( + // (next, context) => async (args) => { + // console.warn('XXX context at build time', context) + // console.warn('XXX args at build time', args) + // return await next(args) + // }, + // { + // step: 'build' + // } + // ) + + // Add a middleware at (or near) the "top", that starts and ends a span. + this.middlewareStack.add( + (next, context) => async (args) => { + console.warn('XXX context: ', context) + console.warn('XXX args: ', args) + + const opName = opNameFromCommandName(context.commandName) + const resource = resourceFromParams(args.input) + const name = resource ? `S3 ${opName} ${resource}` : `S3 ${opName}` + const span = agent.startSpan(name, TYPE, SUBTYPE, opName) + + let err + let result + try { + result = await next(args) + } catch (ex) { + err = ex + } finally { + // XXX I don't think this catches error path (e.g. with ExpiredToken). try/catch for that?! + if (span) { + if (err) { + // XXX captureError, depending in statusCode again + } + + // XXX region technique from ruby might be good here: + // - if client.conf.useArnRegion, then get from Arn if it is, else fallback to client.conf.region? + console.warn('XXX result', result) + // console.warn('XXX client.config.region', client.config.region()) + const region = await client.config.region() + console.warn('XXX region:', region) + + // XXX Destination context. + // const endpoint = request.httpRequest.endpoint + // const destContext = { + // address: endpoint.hostname, + // port: endpoint.port, + // service: { + // name: SUBTYPE, + // type: TYPE + // } + // } + // if (resource) { + // destContext.service.resource = resource + // } + // if (region) { + // destContext.cloud = { region } + // } + // span.setDestinationContext(destContext) + + span.end() + } + } + return result + }, + { + step: 'initialize', + priority: 'high', + name: 'elasticAPMMiddleware' + } + ) + } + } + + agent.logger.debug('shimming <@aws-sdk/client-s3>.S3Client') + + // How do I wrap thee? Let me count the ways. + // + // Ideally we would like to return the loaded `@aws-sdk/client-s3` module, + // `mod`, with the only change being to the `mod.S3Client` constructor; or to + // just replace it with our `ApmS3Client` subclass. However, there are + // roadblocks and options: + // + // 1. The technique used in `@elastic/elasticsearch` instrumentation: + // return Object.assign(mod, { S3Client: ApmS3Client }) + // hits: + // TypeError: Cannot set property S3Client of # which has only a getter + // because the TypeScript-built CommonJS package sets all the exported + // properties to getters for lazy loading. + // + // 2. We cannot just replace the getter with our own: + // Object.defineProperty(mod, 'S3Client', { enumerable: true, get: function () { return ApmS3Client } }) + // return mod + // Because the @aws-sdk/client-s3/dist/cjs/index.js sets 'S3Client' to a + // property with `configurable: false`. + // TypeError: Cannot redefine property: S3Client + // + // 3. We cannot: + // shimmer.wrap(mod, 'S3Client', ApmS3Client) + // for the same reason as #2. + // + // 4. We *can* workaround by shimming the `.send` method: + // shimmer.wrap(AWS.Request.prototype, 'send', function (orig) { ... }) + // but that is not ideal because we then will be attempting to add our + // middleware for every API call rather than just at client creation. + // Still, it would work. + // + // shimmer.wrap(mod.S3Client.prototype, 'send', function (orig) { + // return function _wrappedS3ClientSend () { + // // ... + // return orig.apply(this, arguments) + // } + // }) + // return mod + // + // 5. Finally, we *can* return a totally new object for the module that uses + // a getter for every property that returns the origin `mod`'s property, + // except `S3Client` for which we use our subclass. Currently `mod` has + // no `Object.getOwnPropertySymbols(mod)`. I'm a little uncertain how to + // treat symbols here if there were any. Is this a little heavy-handed? + const wrappedMod = {} + const names = Object.getOwnPropertyNames(mod) + for (let i = 0; i < names.length; i++) { + const name = names[i] + if (name === 'S3Client') { + wrappedMod[name] = ApmS3Client + } else { + Object.defineProperty(wrappedMod, name, { enumerable: true, get: function () { return mod[name] } }) + } + } + return wrappedMod +} diff --git a/package.json b/package.json index b75e0cee7f..78cbb80f4c 100644 --- a/package.json +++ b/package.json @@ -114,6 +114,7 @@ "unicode-byte-truncate": "^1.0.0" }, "devDependencies": { + "@aws-sdk/client-s3": "^3.18.0", "@babel/cli": "^7.8.4", "@babel/core": "^7.8.4", "@babel/preset-env": "^7.8.4", From cec0575d02ae52f7ea1aa2dbe4c5f25d4ff3a6cd Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Fri, 18 Jun 2021 13:53:09 -0700 Subject: [PATCH 05/25] avoid 'disableInstrumentations' test of @aws-sdk/client-s3 on node Date: Fri, 18 Jun 2021 16:39:26 -0700 Subject: [PATCH 06/25] v3 SDK: destination context, error capture, outcome --- .../modules/@aws-sdk/client-s3.js | 113 ++++++++++-------- 1 file changed, 60 insertions(+), 53 deletions(-) diff --git a/lib/instrumentation/modules/@aws-sdk/client-s3.js b/lib/instrumentation/modules/@aws-sdk/client-s3.js index e72437a83c..9c7e998af8 100644 --- a/lib/instrumentation/modules/@aws-sdk/client-s3.js +++ b/lib/instrumentation/modules/@aws-sdk/client-s3.js @@ -5,6 +5,8 @@ // // XXX doc technique used +const constants = require('../../../constants') + const TYPE = 'storage' const SUBTYPE = 's3' @@ -56,36 +58,9 @@ module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabl const client = this - this.middlewareStack.add( - (next, context) => async (args) => { - console.warn('XXX context at finalizeRequest time', context) - console.warn('XXX args at finalizeRequest time', args) - // XXX set destination context here, - // what was `hostname: 's3.us-west-1.amazonaws.com'` at build - // time is now `hostname: 'trentm-play-s3-bukkit2.s3.us-west-1.amazonaws.com',` - return await next(args) - }, - { - step: 'finalizeRequest' - } - ) - // this.middlewareStack.add( - // (next, context) => async (args) => { - // console.warn('XXX context at build time', context) - // console.warn('XXX args at build time', args) - // return await next(args) - // }, - // { - // step: 'build' - // } - // ) - // Add a middleware at (or near) the "top", that starts and ends a span. this.middlewareStack.add( (next, context) => async (args) => { - console.warn('XXX context: ', context) - console.warn('XXX args: ', args) - const opName = opNameFromCommandName(context.commandName) const resource = resourceFromParams(args.input) const name = resource ? `S3 ${opName} ${resource}` : `S3 ${opName}` @@ -96,42 +71,61 @@ module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabl try { result = await next(args) } catch (ex) { + // Save the error for use in `finally` below, but re-throw it to + // not impact code flow. err = ex + throw ex } finally { - // XXX I don't think this catches error path (e.g. with ExpiredToken). try/catch for that?! if (span) { - if (err) { - // XXX captureError, depending in statusCode again + let statusCode + if (result && result.response) { + statusCode = result.response.statusCode + } else if (err && err.$metadata && err.$metadata.httpStatusCode) { + // This code path happens with a conditional GetObject request + // that returns a 304 Not Modified. + statusCode = err.$metadata.httpStatusCode + } + if (statusCode) { + span._setOutcomeFromHttpStatusCode(statusCode) + } else { + span._setOutcomeFromErrorCapture(constants.OUTCOME_FAILURE) + } + if (err && (!statusCode || statusCode >= 400)) { + agent.captureError(err, { skipOutcome: true }) } - // XXX region technique from ruby might be good here: - // - if client.conf.useArnRegion, then get from Arn if it is, else fallback to client.conf.region? - console.warn('XXX result', result) - // console.warn('XXX client.config.region', client.config.region()) + // Destination context. + let port = context._port + if (port === undefined) { + if (context._protocol === 'https:') { + port = 443 + } else if (context._protocol === 'http:') { + port = 80 + } + } + const destContext = { + address: context._hostname, + port: port, + service: { + name: SUBTYPE, + type: TYPE + } + } + if (resource) { + destContext.service.resource = resource + } + // XXX if client.conf.useArnRegion, then get from Arn if it is, else fallback to client.conf.region? + // XXX to try: client using us-east-2, ARN to us-west-1, does it work? const region = await client.config.region() - console.warn('XXX region:', region) - - // XXX Destination context. - // const endpoint = request.httpRequest.endpoint - // const destContext = { - // address: endpoint.hostname, - // port: endpoint.port, - // service: { - // name: SUBTYPE, - // type: TYPE - // } - // } - // if (resource) { - // destContext.service.resource = resource - // } - // if (region) { - // destContext.cloud = { region } - // } - // span.setDestinationContext(destContext) + if (region) { + destContext.cloud = { region } + } + span.setDestinationContext(destContext) span.end() } } + console.warn('XXX returning result') return result }, { @@ -140,6 +134,19 @@ module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabl name: 'elasticAPMMiddleware' } ) + + this.middlewareStack.add( + (next, context) => async (args) => { + // Stash info needed for destination context. + context._hostname = args.request.hostname + context._port = args.request.port + context._protocol = args.request.protocol + return await next(args) + }, + { + step: 'finalizeRequest' + } + ) } } From 26310baba2f699c348aaa3c139d8e34c834b637f Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Mon, 21 Jun 2021 12:30:24 -0700 Subject: [PATCH 07/25] clean up TODOs in the s3 instrumentations --- .../modules/@aws-sdk/client-s3.js | 87 ++++++++++++------- lib/instrumentation/modules/aws-sdk/s3.js | 81 ++++++++++------- 2 files changed, 103 insertions(+), 65 deletions(-) diff --git a/lib/instrumentation/modules/@aws-sdk/client-s3.js b/lib/instrumentation/modules/@aws-sdk/client-s3.js index 9c7e998af8..093f4f7ac3 100644 --- a/lib/instrumentation/modules/@aws-sdk/client-s3.js +++ b/lib/instrumentation/modules/@aws-sdk/client-s3.js @@ -3,12 +3,18 @@ // Instrument the @aws-sdk/client-s3 package. // https://github.com/aws/aws-sdk-js-v3/tree/main/clients/client-s3 // -// XXX doc technique used +// V3 of the JavaScript AWS SDK, '@aws-sdk/client-*', changed its internals +// significantly from v2, 'aws-sdk'. The events on a `AWS.Request` object are +// no longer available for tracing. The technique used for tracing is the new +// "middleware": +// https://aws.amazon.com/blogs/developer/middleware-stack-modular-aws-sdk-js/ +// We instrument by adding our own middleware to created S3Client. const constants = require('../../../constants') const TYPE = 'storage' const SUBTYPE = 's3' +const elasticAPMStash = Symbol('elasticAPMStash') // For a HeadObject API call, `context.commandName === 'HeadObjectCommand'`. const COMMAND_NAME_RE = /^(\w+)Command$/ @@ -21,23 +27,29 @@ function opNameFromCommandName (commandName) { } } -// This regex adapted from "OutpostArn" pattern at -// https://docs.aws.amazon.com/outposts/latest/APIReference/API_Outpost.html -const ARN_REGEX = /^arn:aws([a-z-]+)?:(s3|s3-outposts):[a-z\d-]+:\d{12}:(.*?)$/ +function regionFromS3Arn (s3Arn) { + return s3Arn.split(':')[3] +} -function resourceFromParams (params) { - let resource = null // The bucket name or normalized Access Point/Outpost ARN. - if (params && params.Bucket) { - resource = params.Bucket +// Return an APM "resource" string for the bucket, Access Point ARN, or Outpost +// ARN. ARNs are normalized to a shorter resource name. +// +// Known ARN patterns: +// - arn:aws:s3:::accesspoint/ +// - arn:aws:s3-outposts:::outpost//bucket/ +// - arn:aws:s3-outposts:::outpost//accesspoint/ +// +// In general that is: +// arn:$partition:$service:$region:$accountId:$resource +// +// This parses using the same "split on colon" used by the JavaScript AWS SDK v3. +// https://github.com/aws/aws-sdk-js-v3/blob/v3.18.0/packages/util-arn-parser/src/index.ts#L14-L37 +function resourceFromBucket (bucket) { + let resource = null + if (bucket) { + resource = bucket if (resource.startsWith('arn:')) { - // This is an Access Point or Outpost ARN, e.g.: - // - arn:aws:s3:region:account-id:accesspoint/resource - // - arn:aws:s3-outposts:::outpost//bucket/ - // - arn:aws:s3-outposts:::outpost//accesspoint/ - const match = ARN_REGEX.exec(resource) - if (match) { - resource = match[3] - } + resource = bucket.split(':').slice(5).join(':') } } return resource @@ -62,7 +74,7 @@ module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabl this.middlewareStack.add( (next, context) => async (args) => { const opName = opNameFromCommandName(context.commandName) - const resource = resourceFromParams(args.input) + const resource = resourceFromBucket(args.input.Bucket) const name = resource ? `S3 ${opName} ${resource}` : `S3 ${opName}` const span = agent.startSpan(name, TYPE, SUBTYPE, opName) @@ -81,7 +93,7 @@ module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabl if (result && result.response) { statusCode = result.response.statusCode } else if (err && err.$metadata && err.$metadata.httpStatusCode) { - // This code path happens with a conditional GetObject request + // This code path happens with a GetObject conditional request // that returns a 304 Not Modified. statusCode = err.$metadata.httpStatusCode } @@ -94,17 +106,26 @@ module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabl agent.captureError(err, { skipOutcome: true }) } + // Configuring `new S3Client({useArnRegion:true})` allows one to + // use an Access Point bucket ARN for a region *other* than the + // one for which the client is configured. Therefore, we attempt + // to get the bucket region from the ARN first. + const useArnRegion = await client.config.useArnRegion() + const region = useArnRegion && args.input.Bucket.startsWith('arn:') + ? regionFromS3Arn(args.input.Bucket) + : await client.config.region() + // Destination context. - let port = context._port + let port = context[elasticAPMStash].port if (port === undefined) { - if (context._protocol === 'https:') { + if (context[elasticAPMStash].protocol === 'https:') { port = 443 - } else if (context._protocol === 'http:') { + } else if (context[elasticAPMStash].protocol === 'http:') { port = 80 } } const destContext = { - address: context._hostname, + address: context[elasticAPMStash].hostname, port: port, service: { name: SUBTYPE, @@ -114,9 +135,7 @@ module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabl if (resource) { destContext.service.resource = resource } - // XXX if client.conf.useArnRegion, then get from Arn if it is, else fallback to client.conf.region? - // XXX to try: client using us-east-2, ARN to us-west-1, does it work? - const region = await client.config.region() + if (region) { destContext.cloud = { region } } @@ -125,26 +144,30 @@ module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabl span.end() } } - console.warn('XXX returning result') return result }, { step: 'initialize', priority: 'high', - name: 'elasticAPMMiddleware' + name: 'elasticAPMSpan' } ) + // Add middleware at the later 'finalizeRequest' stage when the regional + // endpoint has been set on `args.request`, and stash that info for + // usage in setting destination context in the middleware above. this.middlewareStack.add( (next, context) => async (args) => { - // Stash info needed for destination context. - context._hostname = args.request.hostname - context._port = args.request.port - context._protocol = args.request.protocol + context[elasticAPMStash] = { + hostname: args.request.hostname, + port: args.request.port, + protocol: args.request.protocol + } return await next(args) }, { - step: 'finalizeRequest' + step: 'finalizeRequest', + name: 'elasticAPMHTTPInfo' } ) } diff --git a/lib/instrumentation/modules/aws-sdk/s3.js b/lib/instrumentation/modules/aws-sdk/s3.js index 571b2ed6af..3f58d4fbe2 100644 --- a/lib/instrumentation/modules/aws-sdk/s3.js +++ b/lib/instrumentation/modules/aws-sdk/s3.js @@ -2,36 +2,53 @@ // Instrument AWS S3 operations via the 'aws-sdk' package. +const constants = require('../../../constants') + const TYPE = 'storage' const SUBTYPE = 's3' -// This regex adapted from "OutpostArn" pattern at -// https://docs.aws.amazon.com/outposts/latest/APIReference/API_Outpost.html -const ARN_REGEX = /^arn:aws([a-z-]+)?:(s3|s3-outposts):[a-z\d-]+:\d{12}:(.*?)$/ +// Return the PascalCase operation name from `request.operation` by undoing to +// `lowerFirst()` from +// https://github.com/aws/aws-sdk-js/blob/c0c44b8a4e607aae521686898f39a3e359f727e4/lib/model/api.js#L63-L65 +// +// For example: 'headBucket' -> 'HeadBucket' +function opNameFromOperation (operation) { + return operation[0].toUpperCase() + operation.slice(1) +} + +// Return an APM "resource" string for the bucket, Access Point ARN, or Outpost +// ARN. ARNs are normalized to a shorter resource name. +// +// Known ARN patterns: +// - arn:aws:s3:::accesspoint/ +// - arn:aws:s3-outposts:::outpost//bucket/ +// - arn:aws:s3-outposts:::outpost//accesspoint/ +// +// In general that is: +// arn:$partition:$service:$region:$accountId:$resource +// +// This parses using the same "split on colon" used by the JavaScript AWS SDK v3. +// https://github.com/aws/aws-sdk-js-v3/blob/v3.18.0/packages/util-arn-parser/src/index.ts#L14-L37 +function resourceFromBucket (bucket) { + let resource = null + if (bucket) { + resource = bucket + if (resource.startsWith('arn:')) { + resource = bucket.split(':').slice(5).join(':') + } + } + return resource +} // Instrument an awk-sdk@2.x operation (i.e. a AWS.Request.send or // AWS.Request.promise). // // @param {AWS.Request} request https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/Request.html function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, enabled }) { - // Get the PascalCase operation name from `request.operation` by undoing to - // `lowerFirst()` from - // https://github.com/aws/aws-sdk-js/blob/c0c44b8a4e607aae521686898f39a3e359f727e4/lib/model/api.js#L63-L65 - const opName = request.operation[0].toUpperCase() + request.operation.slice(1) + const opName = opNameFromOperation(request.operation) let name = 'S3 ' + opName - let resource = null // The bucket name or normalized Access Point/Outpost ARN. - if (request.params && request.params.Bucket) { - resource = request.params.Bucket - if (resource.startsWith('arn:')) { - // This is an Access Point or Outpost ARN, e.g.: - // - arn:aws:s3:region:account-id:accesspoint/resource - // - arn:aws:s3-outposts:::outpost//bucket/ - // - arn:aws:s3-outposts:::outpost//accesspoint/ - const match = ARN_REGEX.exec(resource) - if (match) { - resource = match[3] - } - } + const resource = resourceFromBucket(request.params && request.params.Bucket) + if (resource) { name += ' ' + resource } @@ -43,7 +60,7 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, // Determining the bucket's region. // `request.httpRequest.region` isn't documented, but the aws-sdk@2 - // lib/services/s3.js will often set it to the bucket's determined region. + // lib/services/s3.js will set it to the bucket's determined region. // This can be asynchronously determined -- e.g. if it differs from the // configured service endpoint region -- so this won't be set until // 'complete'. @@ -69,24 +86,23 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, } span.setDestinationContext(destContext) - // XXX I think the following onComplete handling could be shared for all AWS instrumentations - - if (response) { // insanity guard + if (response) { // Follow the spec for HTTP client span outcome. // https://github.com/elastic/apm/blob/master/specs/agents/tracing-instrumentation-http.md#outcome // // For example, a S3 GetObject conditional request (e.g. using the // IfNoneMatch param) will respond with response.error=NotModifed and // statusCode=304. This is a *successful* outcome. - const statusCode = response.httpResponse.statusCode - span._setOutcomeFromHttpStatusCode(statusCode) + const statusCode = response.httpResponse && response.httpResponse.statusCode + if (statusCode) { + span._setOutcomeFromHttpStatusCode(statusCode) + } else { + // `statusCode` will be undefined for errors before sending a request, e.g.: + // InvalidConfiguration: Custom endpoint is not compatible with access point ARN + span._setOutcomeFromErrorCapture(constants.OUTCOME_FAILURE) + } - if (response.error && statusCode && statusCode >= 400) { - // `skipOutcome` because (a) we have set it manually per the - // statusCode, and (b) `captureError` uses `this.currentSpan` which - // currently *gets the wrong span* -- it gets the HTTP span created - // in the same async op for the underlying HTTP request to the AWS - // endpoint. + if (response.error && (!statusCode || statusCode >= 400)) { agent.captureError(response.error, { skipOutcome: true }) } } @@ -97,7 +113,6 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, // an HTTP span created (for this S3 request) in the same async op. That // HTTP span becomes the "active span" for this async op, and *it* gets // marked as sync=false in `before()` in async-hooks.js. - // TODO: move this to a separate issue. span.sync = false span.end() From 0c9d3be72a6e7fc9607df179e3ebc1237714daf8 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 09:51:29 -0700 Subject: [PATCH 08/25] first crack at testing S3 aws-sdk instrumentation using localstack --- .ci/docker/docker-compose-all.yml | 8 + .ci/docker/docker-compose-edge.yml | 8 + .ci/docker/docker-compose-localstack.yml | 14 + .ci/docker/docker-compose-node-edge-test.yml | 1 + .ci/docker/docker-compose-node-test.yml | 1 + .ci/docker/docker-compose.yml | 20 ++ .ci/scripts/test.sh | 3 + package.json | 1 + test/docker-compose.ci.yml | 5 +- test/docker-compose.yml | 26 +- .../aws-sdk/fixtures/use-s3-callback-style.js | 226 +++++++++++++++ .../modules/aws-sdk/s3.test.js | 263 ++++++++++++++++++ 12 files changed, 572 insertions(+), 4 deletions(-) create mode 100644 .ci/docker/docker-compose-localstack.yml create mode 100644 test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js create mode 100644 test/instrumentation/modules/aws-sdk/s3.test.js diff --git a/.ci/docker/docker-compose-all.yml b/.ci/docker/docker-compose-all.yml index 145221ee6a..7727d2c7c5 100644 --- a/.ci/docker/docker-compose-all.yml +++ b/.ci/docker/docker-compose-all.yml @@ -33,6 +33,10 @@ services: extends: file: docker-compose.yml service: memcached + localstack: + extends: + file: docker-compose.yml + service: localstack node_tests: extends: file: docker-compose-node-test.yml @@ -54,6 +58,8 @@ services: condition: service_healthy memcached: condition: service_healthy + localstack: + condition: service_healthy volumes: nodepgdata: @@ -68,3 +74,5 @@ volumes: driver: local nodecassandradata: driver: local + nodelocalstackdata: + driver: local diff --git a/.ci/docker/docker-compose-edge.yml b/.ci/docker/docker-compose-edge.yml index 8a4535dea0..feb27dd063 100644 --- a/.ci/docker/docker-compose-edge.yml +++ b/.ci/docker/docker-compose-edge.yml @@ -9,6 +9,10 @@ services: extends: file: docker-compose.yml service: elasticsearch + localstack: + extends: + file: docker-compose.yml + service: localstack memcached: extends: file: docker-compose.yml @@ -42,6 +46,8 @@ services: condition: service_healthy elasticsearch: condition: service_healthy + localstack: + condition: service_healthy memcached: condition: service_healthy mongodb: @@ -64,6 +70,8 @@ volumes: driver: local nodemysqldata: driver: local + nodelocalstackdata: + driver: local nodeesdata: driver: local nodecassandradata: diff --git a/.ci/docker/docker-compose-localstack.yml b/.ci/docker/docker-compose-localstack.yml new file mode 100644 index 0000000000..a13d7cc0ff --- /dev/null +++ b/.ci/docker/docker-compose-localstack.yml @@ -0,0 +1,14 @@ +version: '2.1' + +services: + localstack: + extends: + file: docker-compose.yml + service: localstack + node_tests: + extends: + file: docker-compose-node-test.yml + service: node_tests + depends_on: + localstack: + condition: service_healthy diff --git a/.ci/docker/docker-compose-node-edge-test.yml b/.ci/docker/docker-compose-node-edge-test.yml index f162da3d37..ad6bac01f7 100644 --- a/.ci/docker/docker-compose-node-edge-test.yml +++ b/.ci/docker/docker-compose-node-edge-test.yml @@ -22,6 +22,7 @@ services: PGHOST: 'postgres' PGUSER: 'postgres' MEMCACHED_HOST: 'memcached' + LOCALSTACK_HOST: 'localstack' NODE_VERSION: ${NODE_VERSION} NVM_NODEJS_ORG_MIRROR: ${NVM_NODEJS_ORG_MIRROR} ELASTIC_APM_ASYNC_HOOKS: ${ELASTIC_APM_ASYNC_HOOKS} diff --git a/.ci/docker/docker-compose-node-test.yml b/.ci/docker/docker-compose-node-test.yml index 94353c5c4e..a2e5403fca 100644 --- a/.ci/docker/docker-compose-node-test.yml +++ b/.ci/docker/docker-compose-node-test.yml @@ -20,6 +20,7 @@ services: PGHOST: 'postgres' PGUSER: 'postgres' MEMCACHED_HOST: 'memcached' + LOCALSTACK_HOST: 'localstack' NODE_VERSION: ${NODE_VERSION} TAV: ${TAV_MODULE} ELASTIC_APM_ASYNC_HOOKS: ${ELASTIC_APM_ASYNC_HOOKS} diff --git a/.ci/docker/docker-compose.yml b/.ci/docker/docker-compose.yml index c18847b987..c31f5649c8 100644 --- a/.ci/docker/docker-compose.yml +++ b/.ci/docker/docker-compose.yml @@ -114,6 +114,24 @@ services: timeout: 10s retries: 5 + localstack: + # https://hub.docker.com/r/localstack/localstack/tags + image: localstack/localstack:0.12.12 + environment: + # XXX See note about TMPDIR on macos at https://github.com/localstack/localstack#using-docker-compose + - "LOCALSTACK_SERVICES=s3" + - "DATA_DIR=/var/lib/localstack" + ports: + - "4566:4566" + healthcheck: + # https://github.com/localstack/localstack/issues/1095#issuecomment-505368702 + test: ["CMD", "awslocal", "s3", "ls"] + interval: 30s + timeout: 10s + retries: 5 + volumes: + - nodelocalstackdata:/var/lib/localstack + volumes: nodepgdata: driver: local @@ -127,3 +145,5 @@ volumes: driver: local nodecassandradata: driver: local + nodelocalstackdata: + driver: local diff --git a/.ci/scripts/test.sh b/.ci/scripts/test.sh index 2881192c7e..6d9495ccb5 100755 --- a/.ci/scripts/test.sh +++ b/.ci/scripts/test.sh @@ -200,6 +200,9 @@ elif [[ -n "${TAV_MODULE}" ]]; then memcached) DOCKER_COMPOSE_FILE=docker-compose-memcached.yml ;; + aws-sdk) + DOCKER_COMPOSE_FILE=docker-compose-localstack.yml + ;; *) # Just the "node_tests" container. No additional services needed for testing. DOCKER_COMPOSE_FILE=docker-compose-node-test.yml diff --git a/package.json b/package.json index 78cbb80f4c..6a8cb62fd9 100644 --- a/package.json +++ b/package.json @@ -184,6 +184,7 @@ "thunky": "^1.1.0", "typescript": "^3.7.5", "untildify": "^4.0.0", + "vasync": "^2.2.0", "wait-on": "^3.3.0", "ws": "^7.2.1" }, diff --git a/test/docker-compose.ci.yml b/test/docker-compose.ci.yml index 4fb6f3cb16..dfe0748a34 100644 --- a/test/docker-compose.ci.yml +++ b/test/docker-compose.ci.yml @@ -3,7 +3,7 @@ version: '2.1' services: node_tests: image: node:${NODE_VERSION} - environment: + environment: MONGODB_HOST: 'mongodb' REDIS_HOST: 'redis' ES_HOST: 'elasticsearch' @@ -11,6 +11,7 @@ services: MYSQL_HOST: 'mysql' CASSANDRA_HOST: 'cassandra' MEMCACHED_HOST: 'memcached' + LOCALSTACK_HOST: 'localstack' PGHOST: 'postgres' PGUSER: 'postgres' depends_on: @@ -30,3 +31,5 @@ services: condition: service_started memcached: condition: service_started + localstack: + condition: service_started diff --git a/test/docker-compose.yml b/test/docker-compose.yml index 00f031628d..bcb4e95d21 100644 --- a/test/docker-compose.yml +++ b/test/docker-compose.yml @@ -4,7 +4,7 @@ services: postgres: user: postgres image: postgres:9.6 - ports: + ports: - "5432:5432" volumes: - nodepgdata:/var/lib/postgresql/data @@ -20,7 +20,7 @@ services: mongodb: image: mongo - ports: + ports: - "27017:27017" volumes: - nodemongodata:/data/db @@ -50,7 +50,7 @@ services: image: mysql:5.7 environment: MYSQL_ALLOW_EMPTY_PASSWORD: 1 - ports: + ports: - "3306:3306" volumes: - nodemysqldata:/var/lib/mysql @@ -114,6 +114,24 @@ services: timeout: 10s retries: 5 + localstack: + # https://hub.docker.com/r/localstack/localstack/tags + image: localstack/localstack:0.12.12 + environment: + # XXX See note about TMPDIR on macos at https://github.com/localstack/localstack#using-docker-compose + - "LOCALSTACK_SERVICES=s3" + - "DATA_DIR=/var/lib/localstack" + ports: + - "4566:4566" + healthcheck: + # https://github.com/localstack/localstack/issues/1095#issuecomment-505368702 + test: ["CMD", "awslocal", "s3", "ls"] + interval: 30s + timeout: 10s + retries: 5 + volumes: + - nodelocalstackdata:/var/lib/localstack + volumes: nodepgdata: driver: local @@ -127,3 +145,5 @@ volumes: driver: local nodecassandradata: driver: local + nodelocalstackdata: + driver: local diff --git a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js new file mode 100644 index 0000000000..afd66a5544 --- /dev/null +++ b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js @@ -0,0 +1,226 @@ +'use strict' + +// Run a single scenario of using the S3 client (callback style) with APM +// enabled. This is used to test that the expected APM events are generated. +// It writes log.info (in ecs-logging format, see +// https://github.com/trentm/go-ecslog#install) for each S3 client API call. +// +// This script can also be used for manual testing of APM instrumentation of S3 +// against a real S3 account. This can be useful because tests are done against +// https://github.com/localstack/localstack that *simulates* S3 with imperfect +// fidelity. +// +// Usage: +// # Run against the default configured AWS profile, creating a new bucket +// # and deleting it afterwards. +// node use-s3-callback-style.js | ecslog +// +// # Testing against localstack. +// docker run --rm -it -e SERVICES=s3 -p 4566:4566 -p 4571:4571 localstack/localstack +// TEST_ENDPOINT=http://localhost:4566 node use-s3-callback-style.js | ecslog +// +// # Use TEST_BUCKET_NAME to re-use an existing bucket (and not delete it). +// # For safety the bucket name must start with "elasticapmtest-bucket-". +// TEST_BUCKET_NAME=elasticapmtest-bucket-1 node use-s3-callback-style.js | ecslog +// +// Output from a sample run is here: +// https://gist.github.com/trentm/c402bcab8c0571f26d879ec0bcf5759c + +const apm = require('../../../../..').start({ + serviceName: 'use-s3-callback-style', + captureExceptions: false, + centralConfig: false, + metricsInterval: 0, + cloudProvider: 'none', + captureSpanStackTraces: false, + stackTraceLimit: 4, // get it smaller for reviewing output + logLevel: 'info' +}) + +const crypto = require('crypto') +const vasync = require('vasync') +const AWS = require('aws-sdk') + +const TEST_BUCKET_NAME_PREFIX = 'elasticapmtest-bucket-' + +// ---- support functions + +// https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html +function useS3 (s3Client, bucketName, cb) { + const region = s3Client.config.region + const log = apm.logger.child({ 'event.module': 'app', bucketName, region }) + const key = 'aDir/aFile.txt' + const content = 'hi there' + + vasync.pipeline({ + arg: {}, + funcs: [ + // Limitation: this doesn't handle paging. + function listAllBuckets (arg, next) { + s3Client.listBuckets({}, function (err, data) { + log.info({ err, data }, 'listBuckets') + if (err) { + next(err) + } else { + arg.bucketIsPreexisting = data.Buckets.some(b => b.Name === bucketName) + next() + } + }) + }, + + // https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#createBucket-property + function createTheBucketIfNecessary (arg, next) { + if (arg.bucketIsPreexisting) { + next() + return + } + + s3Client.createBucket({ + Bucket: bucketName, + CreateBucketConfiguration: { + LocationConstraint: region + } + }, function (err, data) { + // E.g. data: {"Location": "http://trentm-play-s3-bukkit2.s3.amazonaws.com/"} + log.info({ err, data }, 'createBucket') + next(err) + }) + }, + + // https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#bucketExists-waiter + function waitForBucketToExist (_, next) { + s3Client.waitFor('bucketExists', { Bucket: bucketName }, function (err, data) { + log.info({ err, data }, 'waitFor bucketExists') + next(err) + }) + }, + + // https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#putObject-property + function createObj (_, next) { + var md5 = crypto.createHash('md5').update(content).digest('base64') + s3Client.putObject({ + Bucket: bucketName, + Key: key, + ContentType: 'text/plain', + Body: content, + ContentMD5: md5 + }, function (err, data) { + // data.ETag should match a hexdigest md5 of body. + log.info({ err, data }, 'putObject') + next(err) + }) + }, + + function waitForObjectToExist (_, next) { + s3Client.waitFor('objectExists', { + Bucket: bucketName, + Key: key + }, function (err, data) { + log.info({ err, data }, 'waitFor objectExists') + next(err) + }) + }, + + // https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#getObject-property + function getObj (_, next) { + s3Client.getObject({ + Bucket: bucketName, + Key: key + }, function (err, data) { + log.info({ err, data }, 'getObject') + next(err) + }) + }, + + function getObjConditionalGet (_, next) { + const md5hex = crypto.createHash('md5').update(content).digest('hex') + const etag = `"${md5hex}"` + s3Client.getObject({ + IfNoneMatch: etag, + Bucket: bucketName, + Key: key + }, function (err, data) { + log.info({ err, data }, 'getObject conditional get') + // Expect a 'NotModified' error, statusCode=304. + if (err && err.code === 'NotModified') { + next() + } else if (err) { + next(err) + } else { + next(new Error('expected NotModified error for conditional request')) + } + }) + }, + + // https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#deleteObject-property + function deleteTheObj (_, next) { + s3Client.deleteObject({ + Bucket: bucketName, + Key: key + }, function (err, data) { + log.info({ err, data }, 'deleteObject') + next(err) + }) + }, + + function deleteTheBucketIfCreatedIt (arg, next) { + if (arg.bucketIsPreexisting) { + next() + return + } + + s3Client.deleteBucket({ + Bucket: bucketName + }, function (err, data) { + log.info({ err, data }, 'deleteBucket') + next(err) + }) + } + ] + }, function (err) { + if (err) { + log.error({ err }, 'unexpected error using S3') + } + cb(err) + }) +} + +// Return a timestamp of the form YYYYMMDDHHMMSS, which can be used in an S3 +// bucket name: +// https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html +function getTimestamp () { + return (new Date()).toISOString().split('.')[0].replace(/[^0-9]/g, '') +} + +// ---- mainline + +function main () { + // Config vars. + const region = process.env.TEST_REGION || 'us-east-2' + const endpoint = process.env.TEST_ENDPOINT || null + const bucketName = process.env.TEST_BUCKET_NAME || TEST_BUCKET_NAME_PREFIX + getTimestamp() + + // Guard against any bucket name being used because we will be creating and + // deleting objects in it, and potentially *deleting* the bucket. + if (!bucketName.startsWith(TEST_BUCKET_NAME_PREFIX)) { + throw new Error(`cannot use bucket name "${bucketName}", it must start with ${TEST_BUCKET_NAME_PREFIX}`) + } + + const s3Client = new AWS.S3({ + apiVersion: '2006-03-01', + region, + endpoint + }) + + // Ensure an APM transaction so spans can happen. + const tx = apm.startTransaction('manual') + useS3(s3Client, bucketName, function (err) { + if (err) { + tx.setOutcome('failure') + } + tx.end() + process.exitCode = err ? 1 : 0 + }) +} + +main() diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js new file mode 100644 index 0000000000..80bf924ed6 --- /dev/null +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -0,0 +1,263 @@ +'use strict' + +// Test S3 instrumentation of the 'aws-sdk' module. +// +// Note that this uses localstack for testing, which mimicks the S3 API but +// isn't identical. Some known limitations: +// - It basically does nothing with regions, so testing bucket region discovery +// isn't possible. +// - AFAIK localstack does not support Access Points, so access point ARNs +// cannot be tested. + +const { execFile } = require('child_process') + +const tape = require('tape') + +// XXX move this to shared +const { MockAPMServer } = require('../../../stacktraces/_mock_apm_server') + +const endpoint = process.env.LOCALSTACK_HOST + ? 'http://' + process.env.LOCALSTACK_HOST + ':4566' // used by docker-compose config + : 'http://localhost:4566' // default to localstack + +// Execute 'node fixtures/use-s3-callback-style.js' and assert APM server gets +// the expected spans. +tape.test('simple S3 usage scenario', function (t) { + const server = new MockAPMServer() + server.start(function (serverUrl) { + execFile( + process.execPath, + ['fixtures/use-s3-callback-style.js'], + { + cwd: __dirname, + timeout: 10000, // sanity guard on the test hanging + env: Object.assign({}, process.env, { + ELASTIC_APM_SERVER_URL: serverUrl, + TEST_BUCKET_NAME: 'elasticapmtest-bucket-1', + TEST_ENDPOINT: endpoint, + TEST_REGION: 'us-east-2' + }) + }, + function done (err, _stdout, _stderr) { + t.error(err, 'use-s3-callback-style.js errored out') + t.ok(server.events[0].metadata, 'APM server got event metadata object') + + // Sort the events by timestamp, then work through each expected span. + const events = server.events.slice(1) + events.sort((a, b) => { + const aTimestamp = (a.transaction || a.span).timestamp + const bTimestamp = (b.transaction || b.span).timestamp + return aTimestamp < bTimestamp ? -1 : 1 + }) + + // First the transaction. + t.ok(events[0].transaction, 'got the transaction') + const tx = events.shift().transaction + + // Currently HTTP spans under each S3 span are included. Eventually + // those will be excluded. Filter those out for now. + const spans = events.map(e => e.span).filter(e => e.subtype !== 'http') + + // Compare some common fields across all spans. + t.equal(spans.filter(s => s.trace_id === tx.trace_id).length, + spans.length, 'all spans have the same trace_id') + t.equal(spans.filter(s => s.transaction_id === tx.id).length, + spans.length, 'all spans have the same transaction_id') + t.equal(spans.filter(s => s.outcome === 'success').length, + spans.length, 'all spans have outcome="success"') + t.equal(spans.filter(s => s.sync === false).length, + spans.length, 'all spans have sync=false') + t.equal(spans.filter(s => s.sample_rate === 1).length, + spans.length, 'all spans have sample_rate=1') + spans.forEach(s => { + // Remove variable and common fields to facilitate t.deepEqual below. + delete s.id + delete s.transaction_id + delete s.parent_id + delete s.trace_id + delete s.timestamp + delete s.duration + delete s.outcome + delete s.sync + delete s.sample_rate + }) + + // Work through each of the pipeline functions (listAppBuckets, + // createTheBucketIfNecessary, ...) in the script: + t.deepEqual(spans.shift(), { + name: 'S3 ListBuckets', + type: 'storage', + subtype: 's3', + action: 'ListBuckets', + context: { + destination: { + address: 'localhost', + port: 4566, + service: { name: 's3', type: 'storage' }, + cloud: { region: 'us-east-2' } + } + } + }, 'listAllBuckets produced expected span') + + t.deepEqual(spans.shift(), { + name: 'S3 CreateBucket elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'CreateBucket', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'createTheBucketIfNecessary produced expected span') + + // XXX Could be a timing issue here. + t.deepEqual(spans.shift(), { + name: 'S3 HeadBucket elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'HeadBucket', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'waitForBucketToExist produced expected span') + + t.deepEqual(spans.shift(), { + name: 'S3 PutObject elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'PutObject', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'createObj produced expected span') + + // XXX Could be a timing issue here. + t.deepEqual(spans.shift(), { + name: 'S3 HeadObject elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'HeadObject', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'waitForObjectToExist produced expected span') + + t.deepEqual(spans.shift(), { + name: 'S3 GetObject elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'GetObject', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'getObj produced expected span') + + t.deepEqual(spans.shift(), { + name: 'S3 GetObject elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'GetObject', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'getObjConditionalGet produced expected span') + + t.deepEqual(spans.shift(), { + name: 'S3 DeleteObject elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'DeleteObject', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'deleteTheObj produced expected span') + + t.deepEqual(spans.shift(), { + name: 'S3 DeleteBucket elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'DeleteBucket', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'deleteTheBucketIfCreatedIt produced expected span') + + t.equal(spans.length, 0, 'all spans accounted for') + + server.close() + t.end() + } + ) + }) +}) From 642b58dd03881fd4de532a25f898a60336d3dd04 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 09:58:28 -0700 Subject: [PATCH 09/25] github actions need to setup localstack as well; drop node 15 testing in GH actions --- .github/workflows/test.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8f26694cfa..e87d5f36b6 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -105,12 +105,21 @@ jobs: volumes: - nodeesdata:/usr/share/elasticsearch/data + localstack: + image: localstack/localstack:0.12.12 + environment: + - "LOCALSTACK_SERVICES=s3" + - "DATA_DIR=/var/lib/localstack" + ports: + - "4566:4566" + volumes: + - nodelocalstackdata:/var/lib/localstack + strategy: matrix: node: - '16' - '16.0' - - '15' - '14' - '14.0' - '12' From 5305b47a69b8ff0b1ec8a5cb81a7c20bb9864e2c Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 10:22:10 -0700 Subject: [PATCH 10/25] correct GH action yaml syntax --- .github/workflows/test.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e87d5f36b6..c7945f482e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -107,9 +107,9 @@ jobs: localstack: image: localstack/localstack:0.12.12 - environment: - - "LOCALSTACK_SERVICES=s3" - - "DATA_DIR=/var/lib/localstack" + env: + LOCALSTACK_SERVICES: 's3' + DATA_DIR: '/var/lib/localstack' ports: - "4566:4566" volumes: From bdb89dfd280285b10cf8b81429e65bf5a9dcdc80 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 10:44:27 -0700 Subject: [PATCH 11/25] fix the following "openssl ... No such file or directory" when exec'ing 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 --- .ci/scripts/docker-test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.ci/scripts/docker-test.sh b/.ci/scripts/docker-test.sh index 5bc312476a..53fe0c0624 100755 --- a/.ci/scripts/docker-test.sh +++ b/.ci/scripts/docker-test.sh @@ -15,7 +15,7 @@ fi # Skip for node v8 because it results in this warning: # 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" fi # Workaround to git <2.7 From 7006d3975adddf484fe2d52a2a358099be11c478 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 10:59:12 -0700 Subject: [PATCH 12/25] debugging CI test failures --- .ci/.jenkins_nodejs.yml | 21 ++++++++++--------- .github/workflows/test.yml | 19 +++++++++-------- .../modules/aws-sdk/s3.test.js | 6 +++++- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/.ci/.jenkins_nodejs.yml b/.ci/.jenkins_nodejs.yml index fd9868b314..ed7caf062a 100644 --- a/.ci/.jenkins_nodejs.yml +++ b/.ci/.jenkins_nodejs.yml @@ -1,12 +1,13 @@ NODEJS_VERSION: - - "16" - - "16.0" - - "15" - - "14" - - "14.0" + #XXX + # - "16" + # - "16.0" + # - "15" + # - "14" + # - "14.0" - "12" - - "12.0" - - "10" - - "10.0" - - "8" - - "8.6" + # - "12.0" + # - "10" + # - "10.0" + # - "8" + # - "8.6" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c7945f482e..db356014ca 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -118,16 +118,17 @@ jobs: strategy: matrix: node: - - '16' - - '16.0' - - '14' - - '14.0' + #XXX + # - '16' + # - '16.0' + # - '14' + # - '14.0' - '12' - - '12.0' - - '10' - - '10.0' - - '8' - - '8.6' + # - '12.0' + # - '10' + # - '10.0' + # - '8' + # - '8.6' runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index 80bf924ed6..436ac3a0aa 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -38,8 +38,12 @@ tape.test('simple S3 usage scenario', function (t) { TEST_REGION: 'us-east-2' }) }, - function done (err, _stdout, _stderr) { + function done (err, stdout, stderr) { t.error(err, 'use-s3-callback-style.js errored out') + if (err) { + t.comment(`use-s3-callback-style stdout: ${stdout}`) + t.comment(`use-s3-callback-style stderr: ${stderr}`) + } t.ok(server.events[0].metadata, 'APM server got event metadata object') // Sort the events by timestamp, then work through each expected span. From fe0b0a16b2746c61c5d501c526e0ac1cd571f050 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 11:31:40 -0700 Subject: [PATCH 13/25] limit tests run for debugging failures; fix AWS auth envvars for test run; better localstack healthcheck --- .ci/docker/docker-compose.yml | 3 +- test/docker-compose.yml | 3 +- .../aws-sdk/fixtures/use-s3-callback-style.js | 8 +++ .../modules/aws-sdk/s3.test.js | 2 + test/test.js | 49 ++++++++++--------- 5 files changed, 37 insertions(+), 28 deletions(-) diff --git a/.ci/docker/docker-compose.yml b/.ci/docker/docker-compose.yml index c31f5649c8..668e156a33 100644 --- a/.ci/docker/docker-compose.yml +++ b/.ci/docker/docker-compose.yml @@ -124,8 +124,7 @@ services: ports: - "4566:4566" healthcheck: - # https://github.com/localstack/localstack/issues/1095#issuecomment-505368702 - test: ["CMD", "awslocal", "s3", "ls"] + test: ["CMD", "curl", "-f", "http://localhost:4566/health"] interval: 30s timeout: 10s retries: 5 diff --git a/test/docker-compose.yml b/test/docker-compose.yml index bcb4e95d21..d2483dd727 100644 --- a/test/docker-compose.yml +++ b/test/docker-compose.yml @@ -124,8 +124,7 @@ services: ports: - "4566:4566" healthcheck: - # https://github.com/localstack/localstack/issues/1095#issuecomment-505368702 - test: ["CMD", "awslocal", "s3", "ls"] + test: ["CMD", "curl", "-f", "http://localhost:4566/health"] interval: 30s timeout: 10s retries: 5 diff --git a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js index afd66a5544..07d6d04dee 100644 --- a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js +++ b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js @@ -10,6 +10,14 @@ // https://github.com/localstack/localstack that *simulates* S3 with imperfect // fidelity. // +// Auth note: By default this uses the AWS profile/configuration from the +// environment. If you do not have that configured (i.e. do not have +// "~/.aws/...") files, then you can still use localstack via setting: +// unset AWS_PROFILE +// export AWS_ACCESS_KEY_ID=fake +// export AWS_SECRET_ACCESS_KEY=fake +// See also: https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-envvars.html +// // Usage: // # Run against the default configured AWS profile, creating a new bucket // # and deleting it afterwards. diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index 436ac3a0aa..3f543f30a8 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -33,6 +33,8 @@ tape.test('simple S3 usage scenario', function (t) { timeout: 10000, // sanity guard on the test hanging env: Object.assign({}, process.env, { ELASTIC_APM_SERVER_URL: serverUrl, + AWS_ACCESS_KEY_ID: 'fake', + AWS_SECRET_ACCESS_KEY: 'fake', TEST_BUCKET_NAME: 'elasticapmtest-bucket-1', TEST_ENDPOINT: endpoint, TEST_REGION: 'us-east-2' diff --git a/test/test.js b/test/test.js index 1468c5dae5..ebadd01639 100644 --- a/test/test.js +++ b/test/test.js @@ -73,32 +73,33 @@ function mapSeries (tasks, handler, cb) { var directories = [ 'test', - 'test/cloud-metadata', - 'test/instrumentation', - 'test/instrumentation/modules', - 'test/instrumentation/modules/@elastic', - 'test/instrumentation/modules/bluebird', - 'test/instrumentation/modules/cassandra-driver', - 'test/instrumentation/modules/express', - 'test/instrumentation/modules/fastify', - 'test/instrumentation/modules/hapi', - 'test/instrumentation/modules/http', - 'test/instrumentation/modules/koa', - 'test/instrumentation/modules/koa-router', - 'test/instrumentation/modules/mysql', - 'test/instrumentation/modules/mysql2', - 'test/instrumentation/modules/pg', - 'test/instrumentation/modules/restify', + // XXX + // 'test/cloud-metadata', + // 'test/instrumentation', + // 'test/instrumentation/modules', + // 'test/instrumentation/modules/@elastic', + // 'test/instrumentation/modules/bluebird', + // 'test/instrumentation/modules/cassandra-driver', + // 'test/instrumentation/modules/express', + // 'test/instrumentation/modules/fastify', + // 'test/instrumentation/modules/hapi', + // 'test/instrumentation/modules/http', + // 'test/instrumentation/modules/koa', + // 'test/instrumentation/modules/koa-router', + // 'test/instrumentation/modules/mysql', + // 'test/instrumentation/modules/mysql2', + // 'test/instrumentation/modules/pg', + // 'test/instrumentation/modules/restify', 'test/instrumentation/modules/aws-sdk', - 'test/integration', - 'test/integration/api-schema', - 'test/lambda', - 'test/metrics', - 'test/redact-secrets', - 'test/sanitize-field-names', - 'test/sourcemaps', + // 'test/integration', + // 'test/integration/api-schema', + // 'test/lambda', + // 'test/metrics', + // 'test/redact-secrets', + // 'test/sanitize-field-names', + // 'test/sourcemaps', 'test/stacktraces', - 'test/uncaught-exceptions' + // 'test/uncaught-exceptions' ] mapSeries(directories, readdir, function (err, directoryFiles) { From d42619bdb86f6e4546fd8d901b7cd5b22dd81252 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 11:36:03 -0700 Subject: [PATCH 14/25] not helping standard --- test/test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test.js b/test/test.js index ebadd01639..d94d2f30dd 100644 --- a/test/test.js +++ b/test/test.js @@ -99,7 +99,7 @@ var directories = [ // 'test/sanitize-field-names', // 'test/sourcemaps', 'test/stacktraces', - // 'test/uncaught-exceptions' + 'test/uncaught-exceptions' ] mapSeries(directories, readdir, function (err, directoryFiles) { From 0adb071c761da54850035d65c0cc917d0638e61c Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 12:00:12 -0700 Subject: [PATCH 15/25] log used endpoint for debugging; add .promise()-style case --- .../aws-sdk/fixtures/use-s3-callback-style.js | 25 ++++++++++++++++++- .../modules/aws-sdk/s3.test.js | 18 +++++++------ test/script/local-deps-start.sh | 3 +++ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js index 07d6d04dee..4006b5a69e 100644 --- a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js +++ b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js @@ -56,7 +56,12 @@ const TEST_BUCKET_NAME_PREFIX = 'elasticapmtest-bucket-' // https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html function useS3 (s3Client, bucketName, cb) { const region = s3Client.config.region - const log = apm.logger.child({ 'event.module': 'app', bucketName, region }) + const log = apm.logger.child({ + 'event.module': 'app', + endpoint: s3Client.config.endpoint, + bucketName, + region + }) const key = 'aDir/aFile.txt' const content = 'hi there' @@ -160,6 +165,24 @@ function useS3 (s3Client, bucketName, cb) { }) }, + function getObjUsingPromise (_, next) { + const req = s3Client.getObject({ + Bucket: bucketName, + Key: key + }).promise() + + req.then( + function onResolve (data) { + log.info({ data }, 'getObject using Promise, resolve') + next() + }, + function onReject (err) { + log.info({ err }, 'getObject using Promise, reject') + next(err) + } + ) + }, + // https://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/S3.html#deleteObject-property function deleteTheObj (_, next) { s3Client.deleteObject({ diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index 3f543f30a8..aec7ccefc0 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -25,20 +25,22 @@ const endpoint = process.env.LOCALSTACK_HOST tape.test('simple S3 usage scenario', function (t) { const server = new MockAPMServer() server.start(function (serverUrl) { + const additionalEnv = { + ELASTIC_APM_SERVER_URL: serverUrl, + AWS_ACCESS_KEY_ID: 'fake', + AWS_SECRET_ACCESS_KEY: 'fake', + TEST_BUCKET_NAME: 'elasticapmtest-bucket-1', + TEST_ENDPOINT: endpoint, + TEST_REGION: 'us-east-2' + } + t.comment('executing test script with this env: ' + JSON.stringify(additionalEnv)) execFile( process.execPath, ['fixtures/use-s3-callback-style.js'], { cwd: __dirname, timeout: 10000, // sanity guard on the test hanging - env: Object.assign({}, process.env, { - ELASTIC_APM_SERVER_URL: serverUrl, - AWS_ACCESS_KEY_ID: 'fake', - AWS_SECRET_ACCESS_KEY: 'fake', - TEST_BUCKET_NAME: 'elasticapmtest-bucket-1', - TEST_ENDPOINT: endpoint, - TEST_REGION: 'us-east-2' - }) + env: Object.assign({}, process.env, additionalEnv) }, function done (err, stdout, stderr) { t.error(err, 'use-s3-callback-style.js errored out') diff --git a/test/script/local-deps-start.sh b/test/script/local-deps-start.sh index 216a1c66a9..2ddd54c96d 100755 --- a/test/script/local-deps-start.sh +++ b/test/script/local-deps-start.sh @@ -7,3 +7,6 @@ cassandra -p /tmp/cassandra.pid &> /tmp/cassandra.log memcached -d -P /tmp/memcached.pid redis-server /usr/local/etc/redis.conf --daemonize yes mysql.server start + +# Note: localstack is not included here because running localstack +# outside of Docker is deprecated/not supported. From 1eb42da628c4ec8f3aecf42e4f7fc1af698e0db6 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 13:49:09 -0700 Subject: [PATCH 16/25] fixes; still debugging CI --- .../modules/@elastic/elasticsearch.js | 3 +++ .../aws-sdk/fixtures/use-s3-callback-style.js | 1 + .../modules/aws-sdk/s3.test.js | 23 +++++++++++++++++-- test/test.js | 4 ++-- 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/test/instrumentation/modules/@elastic/elasticsearch.js b/test/instrumentation/modules/@elastic/elasticsearch.js index 5627392b5e..bc0f3f5adc 100644 --- a/test/instrumentation/modules/@elastic/elasticsearch.js +++ b/test/instrumentation/modules/@elastic/elasticsearch.js @@ -38,6 +38,9 @@ const pkgVersion = require('@elastic/elasticsearch/package.json').version test('client.ping with promise', function userLandCode (t) { resetAgent(checkDataAndEnd(t, 'HEAD', '/', null)) + t.comment('XXX ES_HOST: ' + process.env.ES_HOST) + t.comment('XXX ES node: ' + node) + agent.startTransaction('myTrans') const client = new Client({ node }) diff --git a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js index 4006b5a69e..3b15a7f299 100644 --- a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js +++ b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js @@ -64,6 +64,7 @@ function useS3 (s3Client, bucketName, cb) { }) const key = 'aDir/aFile.txt' const content = 'hi there' + log.info('XXX started') vasync.pipeline({ arg: {}, diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index aec7ccefc0..faadaa93dd 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -45,8 +45,8 @@ tape.test('simple S3 usage scenario', function (t) { function done (err, stdout, stderr) { t.error(err, 'use-s3-callback-style.js errored out') if (err) { - t.comment(`use-s3-callback-style stdout: ${stdout}`) - t.comment(`use-s3-callback-style stderr: ${stderr}`) + t.comment(`use-s3-callback-style stdout:\n${stdout}\n`) + t.comment(`use-s3-callback-style stderr:\n${stderr}\n`) } t.ok(server.events[0].metadata, 'APM server got event metadata object') @@ -223,6 +223,25 @@ tape.test('simple S3 usage scenario', function (t) { } }, 'getObjConditionalGet produced expected span') + t.deepEqual(spans.shift(), { + name: 'S3 GetObject elasticapmtest-bucket-1', + type: 'storage', + subtype: 's3', + action: 'GetObject', + context: { + destination: { + address: 'elasticapmtest-bucket-1.localhost', + port: 4566, + service: { + name: 's3', + type: 'storage', + resource: 'elasticapmtest-bucket-1' + }, + cloud: { region: 'us-east-2' } + } + } + }, 'getObjUsingPromise produced expected span') + t.deepEqual(spans.shift(), { name: 'S3 DeleteObject elasticapmtest-bucket-1', type: 'storage', diff --git a/test/test.js b/test/test.js index d94d2f30dd..50f2f85c19 100644 --- a/test/test.js +++ b/test/test.js @@ -72,8 +72,8 @@ function mapSeries (tasks, handler, cb) { } var directories = [ - 'test', // XXX + // 'test', // 'test/cloud-metadata', // 'test/instrumentation', // 'test/instrumentation/modules', @@ -98,7 +98,7 @@ var directories = [ // 'test/redact-secrets', // 'test/sanitize-field-names', // 'test/sourcemaps', - 'test/stacktraces', + // 'test/stacktraces', 'test/uncaught-exceptions' ] From 585206ad6f318f72ef8c036029cd9eb3aabb5225 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 15:24:31 -0700 Subject: [PATCH 17/25] 's3ForcePathStyle: true' to fix the bucketName.localstack DNS resolution issue in CI --- .ci/docker/docker-compose.yml | 5 ++-- .ci/scripts/test.sh | 2 ++ .../aws-sdk/fixtures/use-s3-callback-style.js | 24 +++++++++++++++-- .../modules/aws-sdk/s3.test.js | 27 +++++++++---------- test/instrumentation/modules/aws-sdk/sqs.js | 1 + 5 files changed, 39 insertions(+), 20 deletions(-) diff --git a/.ci/docker/docker-compose.yml b/.ci/docker/docker-compose.yml index 668e156a33..76d24116af 100644 --- a/.ci/docker/docker-compose.yml +++ b/.ci/docker/docker-compose.yml @@ -118,9 +118,8 @@ services: # https://hub.docker.com/r/localstack/localstack/tags image: localstack/localstack:0.12.12 environment: - # XXX See note about TMPDIR on macos at https://github.com/localstack/localstack#using-docker-compose - - "LOCALSTACK_SERVICES=s3" - - "DATA_DIR=/var/lib/localstack" + - LOCALSTACK_SERVICES=s3 + - DATA_DIR=/var/lib/localstack ports: - "4566:4566" healthcheck: diff --git a/.ci/scripts/test.sh b/.ci/scripts/test.sh index 6d9495ccb5..26af1d4ee6 100755 --- a/.ci/scripts/test.sh +++ b/.ci/scripts/test.sh @@ -213,6 +213,8 @@ else DOCKER_COMPOSE_FILE=docker-compose-all.yml fi +ELASTIC_APM_ASYNC_HOOKS=${ELASTIC_APM_ASYNC_HOOKS:-true} + set +e NVM_NODEJS_ORG_MIRROR=${NVM_NODEJS_ORG_MIRROR} \ ELASTIC_APM_ASYNC_HOOKS=${ELASTIC_APM_ASYNC_HOOKS} \ diff --git a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js index 3b15a7f299..99b5524fcf 100644 --- a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js +++ b/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js @@ -64,7 +64,6 @@ function useS3 (s3Client, bucketName, cb) { }) const key = 'aDir/aFile.txt' const content = 'hi there' - log.info('XXX started') vasync.pipeline({ arg: {}, @@ -241,7 +240,28 @@ function main () { const s3Client = new AWS.S3({ apiVersion: '2006-03-01', region, - endpoint + endpoint, + // In Jenkins CI the endpoint is "http://localstack:4566", which points to + // a "localstack" docker container on the same network as the container + // running tests. The aws-sdk S3 client defaults to "bucket style" URLs, + // i.e. "http://$bucketName.localstack:4566/$key". This breaks with: + // UnknownEndpoint: Inaccessible host: `mahbukkit.localstack'. This service may not be available in the `us-east-2' region. + // at Request.ENOTFOUND_ERROR (/app/node_modules/aws-sdk/lib/event_listeners.js:530:46) + // ... + // originalError: Error: getaddrinfo ENOTFOUND mahbukkit.localstack + // at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:66:26) { + // errno: 'ENOTFOUND', + // code: 'NetworkingError', + // syscall: 'getaddrinfo', + // hostname: 'mahbukkit.localstack', + // + // It *works* with common localstack usage where the endpoint uses + // *localhost*, because "$subdomain.localhost" DNS resolution still resolves + // to 127.0.0.1. + // + // The work around is to force the client to use "path-style" URLs, e.g.: + // http://localstack:4566/$bucketName/$key + s3ForcePathStyle: true }) // Ensure an APM transaction so spans can happen. diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index faadaa93dd..645a190a0b 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -16,9 +16,8 @@ const tape = require('tape') // XXX move this to shared const { MockAPMServer } = require('../../../stacktraces/_mock_apm_server') -const endpoint = process.env.LOCALSTACK_HOST - ? 'http://' + process.env.LOCALSTACK_HOST + ':4566' // used by docker-compose config - : 'http://localhost:4566' // default to localstack +const LOCALSTACK_HOST = process.env.LOCALSTACK_HOST || 'localhost' +const endpoint = 'http://' + LOCALSTACK_HOST + ':4566' // Execute 'node fixtures/use-s3-callback-style.js' and assert APM server gets // the expected spans. @@ -99,7 +98,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'ListBuckets', context: { destination: { - address: 'localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', type: 'storage' }, cloud: { region: 'us-east-2' } @@ -114,7 +113,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'CreateBucket', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', @@ -126,7 +125,6 @@ tape.test('simple S3 usage scenario', function (t) { } }, 'createTheBucketIfNecessary produced expected span') - // XXX Could be a timing issue here. t.deepEqual(spans.shift(), { name: 'S3 HeadBucket elasticapmtest-bucket-1', type: 'storage', @@ -134,7 +132,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'HeadBucket', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', @@ -153,7 +151,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'PutObject', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', @@ -165,7 +163,6 @@ tape.test('simple S3 usage scenario', function (t) { } }, 'createObj produced expected span') - // XXX Could be a timing issue here. t.deepEqual(spans.shift(), { name: 'S3 HeadObject elasticapmtest-bucket-1', type: 'storage', @@ -173,7 +170,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'HeadObject', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', @@ -192,7 +189,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'GetObject', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', @@ -211,7 +208,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'GetObject', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', @@ -230,7 +227,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'GetObject', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', @@ -249,7 +246,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'DeleteObject', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', @@ -268,7 +265,7 @@ tape.test('simple S3 usage scenario', function (t) { action: 'DeleteBucket', context: { destination: { - address: 'elasticapmtest-bucket-1.localhost', + address: LOCALSTACK_HOST, port: 4566, service: { name: 's3', diff --git a/test/instrumentation/modules/aws-sdk/sqs.js b/test/instrumentation/modules/aws-sdk/sqs.js index 872cd9b926..c2d8a27309 100644 --- a/test/instrumentation/modules/aws-sdk/sqs.js +++ b/test/instrumentation/modules/aws-sdk/sqs.js @@ -2,6 +2,7 @@ const agent = require('../../../..').start({ serviceName: 'test', secretToken: 'test', + cloudProvider: 'none', captureExceptions: false, metricsInterval: 0, centralConfig: false From e8067e4097b1393ef1685080d5ca2e4569570712 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 15:40:13 -0700 Subject: [PATCH 18/25] windows test fix --- .ci/Jenkinsfile | 1 + 1 file changed, 1 insertion(+) diff --git a/.ci/Jenkinsfile b/.ci/Jenkinsfile index 65d0125880..fbd95f4778 100644 --- a/.ci/Jenkinsfile +++ b/.ci/Jenkinsfile @@ -460,6 +460,7 @@ def generateStepForWindows(Map params = [:]){ "ELASTIC_APM_ASYNC_HOOKS=${ELASTIC_APM_ASYNC_HOOKS}", "CASSANDRA_HOST=${linuxIp}", "ES_HOST=${linuxIp}", + "LOCALSTACK_HOST=${linuxIp}", "MEMCACHED_HOST=${linuxIp}", "MONGODB_HOST=${linuxIp}", "MSSQL_HOST=${linuxIp}", From b157b254e0f01b412a90a0253eb047b08bdfa423 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 15:40:22 -0700 Subject: [PATCH 19/25] cleaning up tests --- test/{stacktraces => }/_mock_apm_server.js | 0 .../modules/@elastic/elasticsearch.js | 3 --- .../{use-s3-callback-style.js => use-s3.js} | 8 ++++---- test/instrumentation/modules/aws-sdk/s3.test.js | 15 +++++++-------- test/stacktraces/stacktraces.test.js | 2 +- 5 files changed, 12 insertions(+), 16 deletions(-) rename test/{stacktraces => }/_mock_apm_server.js (100%) rename test/instrumentation/modules/aws-sdk/fixtures/{use-s3-callback-style.js => use-s3.js} (97%) diff --git a/test/stacktraces/_mock_apm_server.js b/test/_mock_apm_server.js similarity index 100% rename from test/stacktraces/_mock_apm_server.js rename to test/_mock_apm_server.js diff --git a/test/instrumentation/modules/@elastic/elasticsearch.js b/test/instrumentation/modules/@elastic/elasticsearch.js index bc0f3f5adc..5627392b5e 100644 --- a/test/instrumentation/modules/@elastic/elasticsearch.js +++ b/test/instrumentation/modules/@elastic/elasticsearch.js @@ -38,9 +38,6 @@ const pkgVersion = require('@elastic/elasticsearch/package.json').version test('client.ping with promise', function userLandCode (t) { resetAgent(checkDataAndEnd(t, 'HEAD', '/', null)) - t.comment('XXX ES_HOST: ' + process.env.ES_HOST) - t.comment('XXX ES node: ' + node) - agent.startTransaction('myTrans') const client = new Client({ node }) diff --git a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js b/test/instrumentation/modules/aws-sdk/fixtures/use-s3.js similarity index 97% rename from test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js rename to test/instrumentation/modules/aws-sdk/fixtures/use-s3.js index 99b5524fcf..aa8e1d2cdc 100644 --- a/test/instrumentation/modules/aws-sdk/fixtures/use-s3-callback-style.js +++ b/test/instrumentation/modules/aws-sdk/fixtures/use-s3.js @@ -21,21 +21,21 @@ // Usage: // # Run against the default configured AWS profile, creating a new bucket // # and deleting it afterwards. -// node use-s3-callback-style.js | ecslog +// node use-s3.js | ecslog // // # Testing against localstack. // docker run --rm -it -e SERVICES=s3 -p 4566:4566 -p 4571:4571 localstack/localstack -// TEST_ENDPOINT=http://localhost:4566 node use-s3-callback-style.js | ecslog +// TEST_ENDPOINT=http://localhost:4566 node use-s3.js | ecslog // // # Use TEST_BUCKET_NAME to re-use an existing bucket (and not delete it). // # For safety the bucket name must start with "elasticapmtest-bucket-". -// TEST_BUCKET_NAME=elasticapmtest-bucket-1 node use-s3-callback-style.js | ecslog +// TEST_BUCKET_NAME=elasticapmtest-bucket-1 node use-s3.js | ecslog // // Output from a sample run is here: // https://gist.github.com/trentm/c402bcab8c0571f26d879ec0bcf5759c const apm = require('../../../../..').start({ - serviceName: 'use-s3-callback-style', + serviceName: 'use-s3', captureExceptions: false, centralConfig: false, metricsInterval: 0, diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index 645a190a0b..55b77e7ed0 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -13,14 +13,13 @@ const { execFile } = require('child_process') const tape = require('tape') -// XXX move this to shared -const { MockAPMServer } = require('../../../stacktraces/_mock_apm_server') +const { MockAPMServer } = require('../../../_mock_apm_server') const LOCALSTACK_HOST = process.env.LOCALSTACK_HOST || 'localhost' const endpoint = 'http://' + LOCALSTACK_HOST + ':4566' -// Execute 'node fixtures/use-s3-callback-style.js' and assert APM server gets -// the expected spans. +// Execute 'node fixtures/use-s3js' and assert APM server gets the expected +// spans. tape.test('simple S3 usage scenario', function (t) { const server = new MockAPMServer() server.start(function (serverUrl) { @@ -35,17 +34,17 @@ tape.test('simple S3 usage scenario', function (t) { t.comment('executing test script with this env: ' + JSON.stringify(additionalEnv)) execFile( process.execPath, - ['fixtures/use-s3-callback-style.js'], + ['fixtures/use-s3.js'], { cwd: __dirname, timeout: 10000, // sanity guard on the test hanging env: Object.assign({}, process.env, additionalEnv) }, function done (err, stdout, stderr) { - t.error(err, 'use-s3-callback-style.js errored out') + t.error(err, 'use-s3.js errored out') if (err) { - t.comment(`use-s3-callback-style stdout:\n${stdout}\n`) - t.comment(`use-s3-callback-style stderr:\n${stderr}\n`) + t.comment(`use-s3.js stdout:\n${stdout}\n`) + t.comment(`use-s3.js stderr:\n${stderr}\n`) } t.ok(server.events[0].metadata, 'APM server got event metadata object') diff --git a/test/stacktraces/stacktraces.test.js b/test/stacktraces/stacktraces.test.js index 7e24a5e6c4..0120d89962 100644 --- a/test/stacktraces/stacktraces.test.js +++ b/test/stacktraces/stacktraces.test.js @@ -9,7 +9,7 @@ const path = require('path') const tape = require('tape') const logging = require('../../lib/logging') -const { MockAPMServer } = require('./_mock_apm_server') +const { MockAPMServer } = require('../_mock_apm_server') const { gatherStackTrace, stackTraceFromErrStackString } = require('../../lib/stacktraces') const log = logging.createLogger('off') From e89a021caec8d44d1ba5c2745779c90da57df0f8 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 16:04:00 -0700 Subject: [PATCH 20/25] restore full node ver testing; modulo dropping node v15 --- .ci/.jenkins_nodejs.yml | 20 +++++++++----------- .github/workflows/test.yml | 19 +++++++++---------- test/docker-compose.yml | 1 - 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/.ci/.jenkins_nodejs.yml b/.ci/.jenkins_nodejs.yml index ed7caf062a..bb31294f62 100644 --- a/.ci/.jenkins_nodejs.yml +++ b/.ci/.jenkins_nodejs.yml @@ -1,13 +1,11 @@ NODEJS_VERSION: - #XXX - # - "16" - # - "16.0" - # - "15" - # - "14" - # - "14.0" + - "16" + - "16.0" + - "14" + - "14.0" - "12" - # - "12.0" - # - "10" - # - "10.0" - # - "8" - # - "8.6" + - "12.0" + - "10" + - "10.0" + - "8" + - "8.6" diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index db356014ca..c7945f482e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -118,17 +118,16 @@ jobs: strategy: matrix: node: - #XXX - # - '16' - # - '16.0' - # - '14' - # - '14.0' + - '16' + - '16.0' + - '14' + - '14.0' - '12' - # - '12.0' - # - '10' - # - '10.0' - # - '8' - # - '8.6' + - '12.0' + - '10' + - '10.0' + - '8' + - '8.6' runs-on: ubuntu-latest steps: - uses: actions/checkout@v2 diff --git a/test/docker-compose.yml b/test/docker-compose.yml index d2483dd727..f007cd8583 100644 --- a/test/docker-compose.yml +++ b/test/docker-compose.yml @@ -118,7 +118,6 @@ services: # https://hub.docker.com/r/localstack/localstack/tags image: localstack/localstack:0.12.12 environment: - # XXX See note about TMPDIR on macos at https://github.com/localstack/localstack#using-docker-compose - "LOCALSTACK_SERVICES=s3" - "DATA_DIR=/var/lib/localstack" ports: From d1239ab7ba07c0bae4eab6ceb6a3a0b9c5f29981 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 16:04:36 -0700 Subject: [PATCH 21/25] restore runing all the test files --- test/test.js | 51 +++++++++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/test/test.js b/test/test.js index 50f2f85c19..1468c5dae5 100644 --- a/test/test.js +++ b/test/test.js @@ -72,33 +72,32 @@ function mapSeries (tasks, handler, cb) { } var directories = [ - // XXX - // 'test', - // 'test/cloud-metadata', - // 'test/instrumentation', - // 'test/instrumentation/modules', - // 'test/instrumentation/modules/@elastic', - // 'test/instrumentation/modules/bluebird', - // 'test/instrumentation/modules/cassandra-driver', - // 'test/instrumentation/modules/express', - // 'test/instrumentation/modules/fastify', - // 'test/instrumentation/modules/hapi', - // 'test/instrumentation/modules/http', - // 'test/instrumentation/modules/koa', - // 'test/instrumentation/modules/koa-router', - // 'test/instrumentation/modules/mysql', - // 'test/instrumentation/modules/mysql2', - // 'test/instrumentation/modules/pg', - // 'test/instrumentation/modules/restify', + 'test', + 'test/cloud-metadata', + 'test/instrumentation', + 'test/instrumentation/modules', + 'test/instrumentation/modules/@elastic', + 'test/instrumentation/modules/bluebird', + 'test/instrumentation/modules/cassandra-driver', + 'test/instrumentation/modules/express', + 'test/instrumentation/modules/fastify', + 'test/instrumentation/modules/hapi', + 'test/instrumentation/modules/http', + 'test/instrumentation/modules/koa', + 'test/instrumentation/modules/koa-router', + 'test/instrumentation/modules/mysql', + 'test/instrumentation/modules/mysql2', + 'test/instrumentation/modules/pg', + 'test/instrumentation/modules/restify', 'test/instrumentation/modules/aws-sdk', - // 'test/integration', - // 'test/integration/api-schema', - // 'test/lambda', - // 'test/metrics', - // 'test/redact-secrets', - // 'test/sanitize-field-names', - // 'test/sourcemaps', - // 'test/stacktraces', + 'test/integration', + 'test/integration/api-schema', + 'test/lambda', + 'test/metrics', + 'test/redact-secrets', + 'test/sanitize-field-names', + 'test/sourcemaps', + 'test/stacktraces', 'test/uncaught-exceptions' ] From 44c860d282526a6dbc67687ffd9c58c4a74e059d Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 16:41:02 -0700 Subject: [PATCH 22/25] drop v3 SDK instrumentation, it has moved to a separate PR --- lib/instrumentation/index.js | 1 - .../modules/@aws-sdk/client-s3.js | 233 ------------------ package.json | 1 - test/config.js | 3 - 4 files changed, 238 deletions(-) delete mode 100644 lib/instrumentation/modules/@aws-sdk/client-s3.js diff --git a/lib/instrumentation/index.js b/lib/instrumentation/index.js index 55dbdef997..fa33f55c7b 100644 --- a/lib/instrumentation/index.js +++ b/lib/instrumentation/index.js @@ -11,7 +11,6 @@ var shimmer = require('./shimmer') var Transaction = require('./transaction') var MODULES = [ - '@aws-sdk/client-s3', '@elastic/elasticsearch', 'apollo-server-core', 'aws-sdk', diff --git a/lib/instrumentation/modules/@aws-sdk/client-s3.js b/lib/instrumentation/modules/@aws-sdk/client-s3.js deleted file mode 100644 index 093f4f7ac3..0000000000 --- a/lib/instrumentation/modules/@aws-sdk/client-s3.js +++ /dev/null @@ -1,233 +0,0 @@ -'use strict' - -// Instrument the @aws-sdk/client-s3 package. -// https://github.com/aws/aws-sdk-js-v3/tree/main/clients/client-s3 -// -// V3 of the JavaScript AWS SDK, '@aws-sdk/client-*', changed its internals -// significantly from v2, 'aws-sdk'. The events on a `AWS.Request` object are -// no longer available for tracing. The technique used for tracing is the new -// "middleware": -// https://aws.amazon.com/blogs/developer/middleware-stack-modular-aws-sdk-js/ -// We instrument by adding our own middleware to created S3Client. - -const constants = require('../../../constants') - -const TYPE = 'storage' -const SUBTYPE = 's3' -const elasticAPMStash = Symbol('elasticAPMStash') - -// For a HeadObject API call, `context.commandName === 'HeadObjectCommand'`. -const COMMAND_NAME_RE = /^(\w+)Command$/ -function opNameFromCommandName (commandName) { - const match = COMMAND_NAME_RE.exec(commandName) - if (match) { - return match[1] - } else { - return '' - } -} - -function regionFromS3Arn (s3Arn) { - return s3Arn.split(':')[3] -} - -// Return an APM "resource" string for the bucket, Access Point ARN, or Outpost -// ARN. ARNs are normalized to a shorter resource name. -// -// Known ARN patterns: -// - arn:aws:s3:::accesspoint/ -// - arn:aws:s3-outposts:::outpost//bucket/ -// - arn:aws:s3-outposts:::outpost//accesspoint/ -// -// In general that is: -// arn:$partition:$service:$region:$accountId:$resource -// -// This parses using the same "split on colon" used by the JavaScript AWS SDK v3. -// https://github.com/aws/aws-sdk-js-v3/blob/v3.18.0/packages/util-arn-parser/src/index.ts#L14-L37 -function resourceFromBucket (bucket) { - let resource = null - if (bucket) { - resource = bucket - if (resource.startsWith('arn:')) { - resource = bucket.split(':').slice(5).join(':') - } - } - return resource -} - -module.exports = function instrumentAwsSdkClientS3 (mod, agent, { version, enabled }) { - if (!enabled) { - return mod - } - if (!mod.S3Client) { - agent.logger.debug('@aws-sdk/client-s3@%s is not supported (no `S3Client`) - aborting...', version) - return mod - } - - class ApmS3Client extends mod.S3Client { - constructor (...args) { - super(...args) - - const client = this - - // Add a middleware at (or near) the "top", that starts and ends a span. - this.middlewareStack.add( - (next, context) => async (args) => { - const opName = opNameFromCommandName(context.commandName) - const resource = resourceFromBucket(args.input.Bucket) - const name = resource ? `S3 ${opName} ${resource}` : `S3 ${opName}` - const span = agent.startSpan(name, TYPE, SUBTYPE, opName) - - let err - let result - try { - result = await next(args) - } catch (ex) { - // Save the error for use in `finally` below, but re-throw it to - // not impact code flow. - err = ex - throw ex - } finally { - if (span) { - let statusCode - if (result && result.response) { - statusCode = result.response.statusCode - } else if (err && err.$metadata && err.$metadata.httpStatusCode) { - // This code path happens with a GetObject conditional request - // that returns a 304 Not Modified. - statusCode = err.$metadata.httpStatusCode - } - if (statusCode) { - span._setOutcomeFromHttpStatusCode(statusCode) - } else { - span._setOutcomeFromErrorCapture(constants.OUTCOME_FAILURE) - } - if (err && (!statusCode || statusCode >= 400)) { - agent.captureError(err, { skipOutcome: true }) - } - - // Configuring `new S3Client({useArnRegion:true})` allows one to - // use an Access Point bucket ARN for a region *other* than the - // one for which the client is configured. Therefore, we attempt - // to get the bucket region from the ARN first. - const useArnRegion = await client.config.useArnRegion() - const region = useArnRegion && args.input.Bucket.startsWith('arn:') - ? regionFromS3Arn(args.input.Bucket) - : await client.config.region() - - // Destination context. - let port = context[elasticAPMStash].port - if (port === undefined) { - if (context[elasticAPMStash].protocol === 'https:') { - port = 443 - } else if (context[elasticAPMStash].protocol === 'http:') { - port = 80 - } - } - const destContext = { - address: context[elasticAPMStash].hostname, - port: port, - service: { - name: SUBTYPE, - type: TYPE - } - } - if (resource) { - destContext.service.resource = resource - } - - if (region) { - destContext.cloud = { region } - } - span.setDestinationContext(destContext) - - span.end() - } - } - return result - }, - { - step: 'initialize', - priority: 'high', - name: 'elasticAPMSpan' - } - ) - - // Add middleware at the later 'finalizeRequest' stage when the regional - // endpoint has been set on `args.request`, and stash that info for - // usage in setting destination context in the middleware above. - this.middlewareStack.add( - (next, context) => async (args) => { - context[elasticAPMStash] = { - hostname: args.request.hostname, - port: args.request.port, - protocol: args.request.protocol - } - return await next(args) - }, - { - step: 'finalizeRequest', - name: 'elasticAPMHTTPInfo' - } - ) - } - } - - agent.logger.debug('shimming <@aws-sdk/client-s3>.S3Client') - - // How do I wrap thee? Let me count the ways. - // - // Ideally we would like to return the loaded `@aws-sdk/client-s3` module, - // `mod`, with the only change being to the `mod.S3Client` constructor; or to - // just replace it with our `ApmS3Client` subclass. However, there are - // roadblocks and options: - // - // 1. The technique used in `@elastic/elasticsearch` instrumentation: - // return Object.assign(mod, { S3Client: ApmS3Client }) - // hits: - // TypeError: Cannot set property S3Client of # which has only a getter - // because the TypeScript-built CommonJS package sets all the exported - // properties to getters for lazy loading. - // - // 2. We cannot just replace the getter with our own: - // Object.defineProperty(mod, 'S3Client', { enumerable: true, get: function () { return ApmS3Client } }) - // return mod - // Because the @aws-sdk/client-s3/dist/cjs/index.js sets 'S3Client' to a - // property with `configurable: false`. - // TypeError: Cannot redefine property: S3Client - // - // 3. We cannot: - // shimmer.wrap(mod, 'S3Client', ApmS3Client) - // for the same reason as #2. - // - // 4. We *can* workaround by shimming the `.send` method: - // shimmer.wrap(AWS.Request.prototype, 'send', function (orig) { ... }) - // but that is not ideal because we then will be attempting to add our - // middleware for every API call rather than just at client creation. - // Still, it would work. - // - // shimmer.wrap(mod.S3Client.prototype, 'send', function (orig) { - // return function _wrappedS3ClientSend () { - // // ... - // return orig.apply(this, arguments) - // } - // }) - // return mod - // - // 5. Finally, we *can* return a totally new object for the module that uses - // a getter for every property that returns the origin `mod`'s property, - // except `S3Client` for which we use our subclass. Currently `mod` has - // no `Object.getOwnPropertySymbols(mod)`. I'm a little uncertain how to - // treat symbols here if there were any. Is this a little heavy-handed? - const wrappedMod = {} - const names = Object.getOwnPropertyNames(mod) - for (let i = 0; i < names.length; i++) { - const name = names[i] - if (name === 'S3Client') { - wrappedMod[name] = ApmS3Client - } else { - Object.defineProperty(wrappedMod, name, { enumerable: true, get: function () { return mod[name] } }) - } - } - return wrappedMod -} diff --git a/package.json b/package.json index 6a8cb62fd9..3df108f47f 100644 --- a/package.json +++ b/package.json @@ -114,7 +114,6 @@ "unicode-byte-truncate": "^1.0.0" }, "devDependencies": { - "@aws-sdk/client-s3": "^3.18.0", "@babel/cli": "^7.8.4", "@babel/core": "^7.8.4", "@babel/preset-env": "^7.8.4", diff --git a/test/config.js b/test/config.js index 9372fee4e9..1ffc197bd2 100644 --- a/test/config.js +++ b/test/config.js @@ -728,9 +728,6 @@ test('disableInstrumentations', function (t) { if (semver.lt(process.version, '10.0.0') && semver.gte(esVersion, '7.12.0')) { modules.delete('@elastic/elasticsearch') } - if (semver.lt(process.version, '10.0.0')) { - modules.delete('@aws-sdk/client-s3') - } function testSlice (t, name, selector) { var selection = selector(modules) From a1892d52f8a7fe77b2f047e656e396130079056f Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 16:46:55 -0700 Subject: [PATCH 23/25] method S3 is supported tech --- docs/supported-technologies.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/supported-technologies.asciidoc b/docs/supported-technologies.asciidoc index 4dfbd148e3..4f2d649241 100644 --- a/docs/supported-technologies.asciidoc +++ b/docs/supported-technologies.asciidoc @@ -72,7 +72,7 @@ The Node.js agent will automatically instrument the following modules to give yo [options="header"] |======================================================================= |Module |Version |Note -|https://www.npmjs.com/package/aws-sdk[aws-sdk] |>1 <3 |Will instrument SQS send/receive/delete messages +|https://www.npmjs.com/package/aws-sdk[aws-sdk] |>1 <3 |Will instrument SQS send/receive/delete messages, all S3 methods |https://www.npmjs.com/package/cassandra-driver[cassandra-driver] |>=3.0.0 |Will instrument all queries |https://www.npmjs.com/package/elasticsearch[elasticsearch] |>=8.0.0 |Will instrument all queries |https://www.npmjs.com/package/@elastic/elasticsearch[@elastic/elasticsearch] |>=7.0.0 <8.0.0 |Will instrument all queries From 54556233c80cf3835dfd9d5e7e52bddbe4c481e2 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 22 Jun 2021 16:55:34 -0700 Subject: [PATCH 24/25] changelog entry --- CHANGELOG.asciidoc | 3 +++ test/instrumentation/modules/aws-sdk/s3.test.js | 1 + 2 files changed, 4 insertions(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 46d2ff5fdc..4ffbbc6aa9 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -36,6 +36,9 @@ Notes: [float] ===== Features +* Add instrumentation of all AWS S3 methods when using the + https://www.npmjs.com/package/aws-sdk[JavaScript AWS SDK v2] (`aws-sdk`). + [float] ===== Bug fixes diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index 55b77e7ed0..678aa721d2 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -62,6 +62,7 @@ tape.test('simple S3 usage scenario', function (t) { // Currently HTTP spans under each S3 span are included. Eventually // those will be excluded. Filter those out for now. + // https://github.com/elastic/apm-agent-nodejs/issues/2125 const spans = events.map(e => e.span).filter(e => e.subtype !== 'http') // Compare some common fields across all spans. From ea9e8ed93972213b6f7f5827d1bdd611417b1814 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Wed, 23 Jun 2021 16:15:08 -0700 Subject: [PATCH 25/25] updates from review feedback --- lib/instrumentation/modules/aws-sdk/s3.js | 10 ++++++---- .../instrumentation/modules/aws-sdk/fixtures/use-s3.js | 2 +- test/instrumentation/modules/aws-sdk/s3.test.js | 6 ++++-- test/script/local-deps-start.sh | 6 ++++-- test/script/local-deps-stop.sh | 1 + 5 files changed, 16 insertions(+), 9 deletions(-) diff --git a/lib/instrumentation/modules/aws-sdk/s3.js b/lib/instrumentation/modules/aws-sdk/s3.js index 3f58d4fbe2..b305adcb16 100644 --- a/lib/instrumentation/modules/aws-sdk/s3.js +++ b/lib/instrumentation/modules/aws-sdk/s3.js @@ -64,20 +64,22 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, // This can be asynchronously determined -- e.g. if it differs from the // configured service endpoint region -- so this won't be set until // 'complete'. - const region = request.httpRequest.region + const region = request.httpRequest && request.httpRequest.region // Destination context. // '.httpRequest.endpoint' might differ from '.service.endpoint' if // the bucket is in a different region. - const endpoint = request.httpRequest.endpoint + const endpoint = request.httpRequest && request.httpRequest.endpoint const destContext = { - address: endpoint.hostname, - port: endpoint.port, service: { name: SUBTYPE, type: TYPE } } + if (endpoint) { + destContext.address = endpoint.hostname + destContext.port = endpoint.port + } if (resource) { destContext.service.resource = resource } diff --git a/test/instrumentation/modules/aws-sdk/fixtures/use-s3.js b/test/instrumentation/modules/aws-sdk/fixtures/use-s3.js index aa8e1d2cdc..6a5778fcb8 100644 --- a/test/instrumentation/modules/aws-sdk/fixtures/use-s3.js +++ b/test/instrumentation/modules/aws-sdk/fixtures/use-s3.js @@ -24,7 +24,7 @@ // node use-s3.js | ecslog // // # Testing against localstack. -// docker run --rm -it -e SERVICES=s3 -p 4566:4566 -p 4571:4571 localstack/localstack +// docker run --rm -it -e SERVICES=s3 -p 4566:4566 localstack/localstack // TEST_ENDPOINT=http://localhost:4566 node use-s3.js | ecslog // // # Use TEST_BUCKET_NAME to re-use an existing bucket (and not delete it). diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index 678aa721d2..f97da6441f 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -51,14 +51,16 @@ tape.test('simple S3 usage scenario', function (t) { // Sort the events by timestamp, then work through each expected span. const events = server.events.slice(1) events.sort((a, b) => { - const aTimestamp = (a.transaction || a.span).timestamp - const bTimestamp = (b.transaction || b.span).timestamp + const aTimestamp = (a.transaction || a.span || {}).timestamp + const bTimestamp = (b.transaction || b.span || {}).timestamp return aTimestamp < bTimestamp ? -1 : 1 }) // First the transaction. t.ok(events[0].transaction, 'got the transaction') const tx = events.shift().transaction + t.equal(events.filter(e => e.span).length, events.length, + 'all remaining events are spans') // Currently HTTP spans under each S3 span are included. Eventually // those will be excluded. Filter those out for now. diff --git a/test/script/local-deps-start.sh b/test/script/local-deps-start.sh index 2ddd54c96d..8cdeb1d18e 100755 --- a/test/script/local-deps-start.sh +++ b/test/script/local-deps-start.sh @@ -8,5 +8,7 @@ memcached -d -P /tmp/memcached.pid redis-server /usr/local/etc/redis.conf --daemonize yes mysql.server start -# Note: localstack is not included here because running localstack -# outside of Docker is deprecated/not supported. +# Note: Running a "local" (i.e. outside of Docker) localstack is deprecated/not +# supported. So we run it in Docker. +docker run --name dev-localstack -d --rm -e SERVICES=s3 -p 4566:4566 localstack/localstack + diff --git a/test/script/local-deps-stop.sh b/test/script/local-deps-stop.sh index cf5cfb14f2..aed2755783 100755 --- a/test/script/local-deps-stop.sh +++ b/test/script/local-deps-stop.sh @@ -7,3 +7,4 @@ kill `cat /tmp/cassandra.pid` kill `cat /tmp/memcached.pid` redis-cli shutdown mysql.server stop +docker stop dev-localstack