-
Notifications
You must be signed in to change notification settings - Fork 797
botocore: Add AWS_LAMBDA_RESOURCE_MAPPING_ID Semantic Convention Support for AWS Lambda SDK #3800
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
base: main
Are you sure you want to change the base?
Conversation
…S Lambda SDK This PR adds support for the AWS_LAMBDA_RESOURCE_MAPPING_ID semantic convention attribute in the AWS Lambda SDK instrumentation library. https://github.com/open-telemetry/semantic-conventions/blob/main/docs/registry/attributes/aws.md#amazon-lambda-attributes It also introduces the following two experimental attributes. Work is currently underway to add these keys to the AWS section of the Semantic Conventions registry: aws.lambda.function.arn aws.lambda.function.name Name is extracted from request object. ARN is extracted from response object. Resource Mapping ID is extracted from both request and response objects. This behavior is covered by unit tests. Tests Added new unit tests (passing). Verified with: tox -e py312-test-instrumentation-botocore tox -e spellcheck tox -e lint-instrumentation-botocore tox -e ruff Backward Compatibility This change is backward compatible. It only adds instrumentation for additional AWS resources and does not modify existing behavior in the auto-instrumentation library.
instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_lambda.py
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_lambda.py
Show resolved
Hide resolved
...metry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/extensions/lmbd.py
Outdated
Show resolved
Hide resolved
bce20a8
to
83747fb
Compare
@lukeina2z @yiyuan-he thanks for your PRs and reviews, what do you think on being listed as component owner for the botocore instrumentation since you seem to have an interest in it? |
@xrmx Thanks Riccardo, I'd be happy come onboard as a component owner! |
...metry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/extensions/lmbd.py
Show resolved
Hide resolved
I am happy to contribute. Let's move forward. |
Thanks, please approve #3806 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
from opentelemetry.trace.span import Span | ||
|
||
# Work is underway to add these two keys to the SemConv AWS registry, in line with other AWS resources. | ||
# https://github.com/open-telemetry/semantic-conventions/blob/main/docs/registry/attributes/aws.md#amazon-lambda-attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually don't add attributes before they're added to semconv, do you want to wait for these to get there or move the usage of these attributes to another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTel Python might have a different policy. I added these two attributes to OTel Java a few months ago:
OTel Java:
open-telemetry/opentelemetry-java-instrumentation#14229
Is there any harm in keeping them in this PR? I can remove them if this is a strict rule for Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aws.lambda.function.name
seems to be redundant as there is already faas.invoked.name, which the instrumentation already captures (at least for Invoke
operations).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed aws.lambda.function.arn and aws.lambda.function.name.
return function_name_or_arn if function_name is None else function_name | ||
if function_name_or_arn is None: | ||
return None | ||
matches = _OpInvoke.ARN_LAMBDA_PATTERN.match(function_name_or_arn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you are moving to a staticmethod while still referencing class attributes but not an issue for me 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I am going to switch it back to @classmethod.
Description
This PR adds support for the AWS_LAMBDA_RESOURCE_MAPPING_ID semantic convention attribute in the AWS Lambda SDK instrumentation library.
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/registry/attributes/aws.md#amazon-lambda-attributes
Resource Mapping ID is extracted from both request and response objects. This behavior is covered by unit tests.
Backward Compatibility
This change is backward compatible. It only adds instrumentation for additional AWS resources and does not modify existing behavior in the auto-instrumentation library.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added new unit tests (passing).
Verified with:
tox -e py39-test-instrumentation-botocore-0
tox -e py39-test-instrumentation-botocore-1
tox -e py310-test-instrumentation-botocore-0
tox -e py310-test-instrumentation-botocore-1
tox -e py311-test-instrumentation-botocore-0
tox -e py311-test-instrumentation-botocore-1
tox -e py312-test-instrumentation-botocore-0
tox -e py312-test-instrumentation-botocore-1
tox -e py313-test-instrumentation-botocore-0
tox -e py313-test-instrumentation-botocore-1
tox -e spellcheck
tox -e lint-instrumentation-botocore
tox -e ruff
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.