Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ALB events in opentelemetry-instrumentation-aws-lambda #2585

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

jbfenton
Copy link
Contributor

@jbfenton jbfenton commented Jun 7, 2024

Description

The current opentelemetry-instrumentation-aws-lambda module only supports conventional API Gateway events which typically contain a headers field that the trace context can be extracted from.

ALB events will only contain a headers field when Multi value headers is disabled for the ALB in question. Otherwise the necessary headers will be found under the 'multiValueHeaders` field:
ELB: Lambda functions as targets

As a part of implementing this change, types + interfaces of the instrumentation have been hardened so that we avoid passing Optional parameters down the call chain wherever it is practical.

Handlers for processing types of events have also been placed in wrappers to make it a little easier to identify where we are pulling the trace context from, and where/why a given set of span attributes is being extracted + applied.

The hope is that in the future if someone wanted to add an additional span attribute for an API Gateway Proxy they could simply add it to APIGatewayProxyWrapper.set_pre_execution_span_attributes without risk of breaking any of the other event handlers.

This change should also resolve: #2511

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
    • Current documentation doesn't cover event_context_extractor that has changed.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Sample events have been added for all types of events that the instrumentation supports.
  • Unit tests have also been aded for new/modified utility functions.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Copy link

linux-foundation-easycla bot commented Jun 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@xrmx
Copy link
Contributor

xrmx commented Jun 7, 2024

@jbfenton need a changelog entry

@jbfenton jbfenton requested a review from a team June 8, 2024 02:37
@jbfenton
Copy link
Contributor Author

jbfenton commented Jun 8, 2024

@jbfenton need a changelog entry

Changelog updated in 393dcf2

@jbfenton jbfenton requested a review from xrmx June 8, 2024 04:30
@jbfenton jbfenton requested a review from zhihali June 17, 2024 00:47
@jbfenton jbfenton force-pushed the improve_lambda_event_payload_support branch from 2d0e287 to 9ee67ca Compare June 21, 2024 01:31
Comment on lines +193 to +196
if not hasattr(tracer_provider, "force_flush"):
tracer_provider.force_flush = provider_warning(
provider_type="TracerProvider"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting all this work and testing in!

I don't quite understand the functionality/conveniences the new FlushableTracerProvider and FlushableMeterProvider provide, since the SDK's versions have force_flush implemented (here and here). If a user or vendor implemented their own by extending the API, would the not hasattr check and set be enough? Or, why not update the API instead so other instrumentors could use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally the Flushable Providers don't serve any purpose other than fixing up some type-hinting issues.
I'll have a look back through and see if it can be optimised.

Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a look at this today, it's great to have the tests with proper mock responses but the amount of code that has been added was intimidating 😅

Would it possible to first add the tests and add whatever minimal change is eventually needed to have them pass and do the refactoring later?

@jbfenton
Copy link
Contributor Author

Take a look at this today, it's great to have the tests with proper mock responses but the amount of code that has been added was intimidating 😅

Would it possible to first add the tests and add whatever minimal change is eventually needed to have them pass and do the refactoring later?

Thanks for taking a look!

I'm happy to split this across a couple of PRs, I'll transition this PR back to draft, and spin up a new PR to introduce tests first.

@jbfenton jbfenton marked this pull request as draft July 31, 2024 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS Lambda event source key incorrect for SNS in instrumentation library.
4 participants