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

adding additional event sources #926

Merged
merged 13 commits into from
Mar 11, 2022

Conversation

brett-bim
Copy link
Contributor

@brett-bim brett-bim commented Feb 18, 2022

Description

Fixes 902. Current behavior is not setting a default for SpanKind in the lambda library. When an event arrives that is not of type SQS, no exception is triggered and span_kind is accessed later when not set.

This PR does two things:

  1. Sets a default of SpanKind.SERVER which seems to be the intent of the existing code.
  2. Expands the concept of consumer beyond SQS to include any event source that might send a message to a lambda. This seems consistent with the idea that the lambda is responding to an event, e.g. consuming that event even though it's not a classic queue based consumer.

Fixes # (issue)
#902

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

  • Create an S3 bucket. Create a Lambda that utilizes the lambda layer for OTel. Subscribe the lambda to the bucket via an event that occurs when files are created in the bucket. Upload a file to the bucket. Verify that telemetry does not occur correctly and instead messages related to span_kind being accessed before it is set.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

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

  • [ NA] Followed the style guidelines of this project
  • [ Yes] Changelogs have been updated.
  • [ Yes] Unit tests have been added
  • [ NA] Documentation has been updated

@brett-bim brett-bim requested a review from a team February 18, 2022 02:47
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 18, 2022

CLA Signed

The committers are authorized under a signed CLA.

# https://docs.aws.amazon.com/lambda/latest/dg/with-sns.html
# https://docs.aws.amazon.com/AmazonS3/latest/userguide/notification-content-structure.html
# https://docs.aws.amazon.com/lambda/latest/dg/with-ddb.html
span_kind = SpanKind.SERVER
Copy link
Member

@srikanthccv srikanthccv Feb 23, 2022

Choose a reason for hiding this comment

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

is there any case when this default is incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now the default is implicitly set to null which doesn't allow the code to even proceed past the usage of span_kind downstream. Seems like the original intent was to look for SQS and call it a consumer if it didn't exist in the map. I'm interested to hear @NathanielRN 's opinion though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this was an oversight on my part, appreciate your deep dive into the code @brett-bim! In general we assumed that we wanted all spans to be Server spans.

(This is important for AWS X-Ray because only Server spans get converted to segments).

@srikanthccv
Copy link
Member

ping @NathanielRN

Copy link
Contributor

@NathanielRN NathanielRN left a comment

Choose a reason for hiding this comment

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

Thanks for this! Just small comments but it's good as is for the fix.

Do you think you could add a test? I thought I had one but I guess I never did for that code path.

The instrumentation/opentelemetry-instrumentation-aws-lambda/tests/test_aws_lambda_instrumentation_manual.py file should be completely set up for your to do that. You can copy a test like the test_parent_context_from_lambda_event test to create a SQS/DynamoDB/etc. test event.

# https://docs.aws.amazon.com/lambda/latest/dg/with-sns.html
# https://docs.aws.amazon.com/AmazonS3/latest/userguide/notification-content-structure.html
# https://docs.aws.amazon.com/lambda/latest/dg/with-ddb.html
span_kind = SpanKind.SERVER
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this was an oversight on my part, appreciate your deep dive into the code @brett-bim! In general we assumed that we wanted all spans to be Server spans.

(This is important for AWS X-Ray because only Server spans get converted to segments).

CHANGELOG.md Outdated Show resolved Hide resolved
@brett-bim
Copy link
Contributor Author

I think I've addressed @NathanielRN 's comments and suggestions.

@brett-bim
Copy link
Contributor Author

Seems that several of the actions failed after I added a test. I was able to execute the test locally with pytest both before my changes and after to prove it worked. As I and several others have noted in the Slack channel, I haven't been able to follow the set up documentation to run tox locally. Not sure how to proceed.

@srikanthccv
Copy link
Member

There were some failures outside your changes. That should be fixed now.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

@brett-bim please sign the cla

@brett-bim
Copy link
Contributor Author

CLA is signed.

@ocelotl
Copy link
Contributor

ocelotl commented Mar 9, 2022

@brett-bim please fix conflicts so we can merge

@brett-bim
Copy link
Contributor Author

@ocelotl fixed in my fork.

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.

lambdas triggered from dynamodb are failing
5 participants