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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Added


- `opentelemetry-instrumentation-aws-lambda` Added additional consumers. Fixes issue 902
brett-bim marked this conversation as resolved.
Show resolved Hide resolved
([#926](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/926))

- `opentelemetry-instrumentation-dbapi` add experimental sql commenter capability
([#908](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/908))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,14 @@ def _instrumented_lambda_handler_call(

# See more:
# https://docs.aws.amazon.com/lambda/latest/dg/with-sqs.html
# 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).

brett-bim marked this conversation as resolved.
Show resolved Hide resolved
valid_consumers = ["aws:sqs", "aws:s3", "aws:sns", "aws:dynamodb"]

try:
if lambda_event["Records"][0]["eventSource"] == "aws:sqs":
if lambda_event["Records"][0]["eventSource"] in valid_consumers:
span_kind = SpanKind.CONSUMER
except (IndexError, KeyError, TypeError):
span_kind = SpanKind.SERVER
brett-bim marked this conversation as resolved.
Show resolved Hide resolved
Expand Down