Skip to content

Add instrumentation for AWS Lambda Service - pkg metadata files (Part 1/2)#739

Merged
owais merged 2 commits intoopen-telemetry:mainfrom
NathanielRN:add-aws_lambda-instrumentation-package
Oct 26, 2021
Merged

Add instrumentation for AWS Lambda Service - pkg metadata files (Part 1/2)#739
owais merged 2 commits intoopen-telemetry:mainfrom
NathanielRN:add-aws_lambda-instrumentation-package

Conversation

@NathanielRN
Copy link
Copy Markdown
Contributor

@NathanielRN NathanielRN commented Oct 14, 2021 •
edited
Loading

Description

In this PR, we ONLY add package metadata files.

This PR adds Instrumentation for Lambda functions on the AWS Lambda service. Specifically, when you use AWS Lambda to call your def lambda_handler(lambda_event, lambda_context) on AWS, you can use this instrumentation package and its scripts to instrument that invocation with OpenTelemetry!

OpenTelemetry already hosts this code on the OpenTelemetry Lambda repository, but it has a TODO to migrate it here, to the OpenTelemetry Python Contrib repository.

Instrumenting AWS Lambda is defined in the OpenTelemetry specifications and "AWS Lambda Instrumentations" also already exist in other languages:

You can learn more about how they are used on the AWS Distro for OpenTelemetry - Python Lambda Layer documentation

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

N/A

Does This PR Require a Core Repo Change?

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

@NathanielRN NathanielRN requested a review from a team October 14, 2021 04:32
@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch 3 times, most recently from b2b0422 to fc943f4 Compare October 14, 2021 05:06
@NathanielRN NathanielRN mentioned this pull request Oct 14, 2021
Merged
3 tasks
@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch from fc943f4 to 0f15526 Compare October 14, 2021 05:11
@NathanielRN NathanielRN mentioned this pull request Oct 14, 2021
Closed
@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch 2 times, most recently from ecebc7a to 6f99ec4 Compare October 14, 2021 05:24
@NathanielRN NathanielRN marked this pull request as draft October 14, 2021 06:32
@NathanielRN NathanielRN mentioned this pull request Oct 21, 2021
Closed
@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch 4 times, most recently from 301d2bc to 3fc7fff Compare October 22, 2021 17:32
@NathanielRN NathanielRN marked this pull request as ready for review October 22, 2021 17:33
@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch from 3fc7fff to 7cb3428 Compare October 22, 2021 17:36
@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch 4 times, most recently from a710b10 to 7c0787b Compare October 22, 2021 19:05
owais
owais reviewed Oct 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

Q: How would this work if the lambda uses a framework like flask or falcon? Wouldn't that result in two SERVER spans?

owais
owais reviewed Oct 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

aiohttp-client uses - as the separate but this PR is using _ i.e, aws_lambda instead of aws-lambda. Can we use aws-lambda instead?

...elemetry-instrumentation-aws_lambda/src/opentelemetry/instrumentation/aws_lambda/__init__.py Outdated
with tracer.start_as_current_span(
name=orig_handler_name,
context=parent_context,
kind=SpanKind.SERVER,
Copy link
Copy Markdown
Contributor

@owais owais Oct 25, 2021

Choose a reason for hiding this comment

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

What if the lambda was triggered by SQS/SNS/Kinesis/S3/etc? It would be a consumer span in that case.

Copy link
Copy Markdown
Contributor Author

@NathanielRN NathanielRN Oct 26, 2021

Choose a reason for hiding this comment

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

Moved this conversation to #777 (comment)

lzchen
lzchen reviewed Oct 25, 2021
View reviewed changes
lzchen
lzchen reviewed Oct 25, 2021
View reviewed changes
@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch from 7c0787b to 2a20f4d Compare October 25, 2021 19:32
@NathanielRN NathanielRN changed the title Add instrumentation for AWS Lambda Service Add instrumentation for AWS Lambda Service - pkg metadata files (Part 1/2) Oct 25, 2021
@NathanielRN NathanielRN mentioned this pull request Oct 25, 2021
Merged
6 tasks
@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch 4 times, most recently from 05a8c46 to 1d984e8 Compare October 25, 2021 22:15
@NathanielRN NathanielRN requested review from lzchen and owais October 25, 2021 22:24
@NathanielRN
Copy link
Copy Markdown
Contributor Author

NathanielRN commented Oct 25, 2021

@owais @lzchen Thank you for your comments! 🙂 I'm moving most of the Python code into #777 to make this PR easy to review. Will address your further questions there!

owais
owais approved these changes Oct 25, 2021
View reviewed changes
@owais
Copy link
Copy Markdown
Contributor

owais commented Oct 25, 2021

Sorry for the trouble but any chance we can convert this into PR number 2 with all the code and create a new PR with the package skeleton? 😬

Only asking because we already started a review on this and it'd be weird to have these discussions in the wrong place. Happy to copy all the comments and add them to another PR though if it is too much trouble.

@NathanielRN NathanielRN mentioned this pull request Oct 25, 2021
Open
@NathanielRN
Copy link
Copy Markdown
Contributor Author

NathanielRN commented Oct 25, 2021

@owais

Only asking because we already started a review on this and it'd be weird to have these discussions in the wrong place. Happy to copy all the comments and add them to another PR though if it is too much trouble.

Oh I was going to copy all your Python code related comments to #777! It's pretty dependent on this PR so I thought it would be best to merge this one first 🙂 If you prefer to make this PR the "Python code" PR we can do that instead though? Let me know!

@owais
Copy link
Copy Markdown
Contributor

owais commented Oct 25, 2021

Whatever is easier for you @NathanielRN

@NathanielRN NathanielRN force-pushed the add-aws_lambda-instrumentation-package branch from 1d984e8 to 657c2b1 Compare October 26, 2021 14:14
@owais owais merged commit dbc6a86 into open-telemetry:main Oct 26, 2021
@NathanielRN NathanielRN deleted the add-aws_lambda-instrumentation-package branch October 26, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@owais owais owais approved these changes

@lzchen lzchen Awaiting requested review from lzchen

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@NathanielRN @owais @lzchen