open-telemetry / opentelemetry-js-contrib

OpenTelemetry instrumentation for JavaScript modules
https://opentelemetry.io
Apache License 2.0
661 stars 487 forks source link

@opentelemetry/instrumentation-aws-lambda should read API gateway parameters by default #999

Open lizthegrey opened 2 years ago

lizthegrey commented 2 years ago

Is your feature request related to a problem? Please describe

Invokes create a root span, but populate only details about the lambda itself (e.g. /usr/bin/node, lambda runtime version, function name) rather than about the request being performed. When the lambda is being called directly from API gateway, these details should automatically be populated, rather than relying upon telemetry from API gateway to have been also plumbed up and amazon trace header used for correlation.

Describe the solution you'd like to see

the following event.input (but not event.body, to avoid PII leaks) values should be automatically extracted and put into attributes (essentially as a default requestHook); example from https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-create-api-as-simple-proxy-for-lambda.html

  "input":{
    "resource":"/helloworld",
    "path":"/helloworld",
    "httpMethod":"POST",
    "headers": {
      "Accept":"*/*",
      "content-type":"application/json",
      "Host":"r275xc9bmd.execute-api.us-east-1.amazonaws.com",
      "User-Agent":"curl/7.64.0",
      "X-Amzn-Trace-Id":"Root=1-1a2b3c4d-a1b2c3d4e5f6a1b2c3d4e5f6",
      "X-Forwarded-For":"72.21.198.64",
      "X-Forwarded-Port":"443",
      "X-Forwarded-Proto":"https"
    },
    "queryStringParameters":{
      "city":"Seattle",
      "name":"John"
    },
  },

Describe alternatives you've considered

These attributes could be left on an API gateway span, except there's no easy way to produce OTel formatted spans out of API gateway logs, and even then, API gateway only speaks amazon trace header, not w3c tracecontext, so you'd need to use amazon propagator.

Additional context

tagging component owners @willarmiros @NathanielRN and tagging Honeycomb PM @cartermp tagging users who encountered this problem and showed me: @longility @nikordaris @meleksomai

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

lizthegrey commented 2 years ago

still is a problem/footgun.

NathanielRN commented 2 years ago

Tagging @vasireddy99 and @bryan-aguilar to triage this for investigation.

dyladan commented 2 years ago

This is not stale. Tagging component owners @willarmiros @NathanielRN

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

cartermp commented 1 year ago

Still not stale. Happy to contribute a fix myself, just unsure where I'd access the kind of information that we should be attaching to the created span.

willarmiros commented 1 year ago

Hi @cartermp,

It seems these additional attributes for APIGW-invoked Lambdas would have to be added in the Lambda instrumentation around here: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/4a9442ceea7f8555ad6e5f731f92834f3398d5b9/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts#L166

You would need to parse the event used to invoke the Lambda to determine if it is an APIGW event, then record the desired parameters if so.

NPellet commented 1 year ago

@willarmiros

In case the lambda is triggered from the Gateway API, should the span status be set to Status.Error if the lambda is returning a statusCode != 2XX ? (See https://docs.aws.amazon.com/apigateway/latest/developerguide/handle-errors-in-lambda-integration.html)

We have Sentry that's catching our thrown exceptions and returning a proper 500, but in that case, the lambda itself doesn't really fail, hence the span is not errored, while we receive a Sentry alert.

Just wondering

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

willarmiros commented 1 year ago

Hi @NPellet - sorry for the delayed response I must have missed this. Yeah I think based on those docs we are only handling the "standard error" case, but not the "custom error" case. IMO if the customer is creating a custom error as described in those docs and we can read the HTTP Status code of that custom error, then it is clear that the customer intent is to mark that request as failed and we should update the span status to Error.

Also commenting to remove stale.

willarmiros commented 1 year ago

@dyladan can we make this a never-stale?

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

blytheaw commented 1 year ago

We are definitely interested in this feature, especially since API Gateway v2 (HTTP API) isn't supported by AWS X-ray and many other third party providers. Having that as the root span would be immensely helpful.