botocore: trace presigned URL generation #4096
Conversation
| wrapped(*args, **kwargs) | ||
|
|
||
| try: | ||
| original = instance.generate_presigned_url |
There was a problem hiding this comment.
We should be using hasattr or getattr here.
| wrap_function_wrapper( | ||
| "botocore.signers", | ||
| "RequestSigner.__init__", | ||
| self._patched_signer_init, | ||
| ) |
There was a problem hiding this comment.
Can we not just do:
| wrap_function_wrapper( | |
| "botocore.signers", | |
| "RequestSigner.__init__", | |
| self._patched_signer_init, | |
| ) | |
| wrap_function_wrapper( | |
| "botocore.signers", | |
| "RequestSigner.generate_presigned_url", | |
| self._patched_generate_presigned_url, | |
| ) |
| tracer = get_tracer(__name__, __version__, self.tracer_provider) | ||
|
|
||
| with tracer.start_as_current_span("botocore.presigned_url") as span: | ||
| try: |
There was a problem hiding this comment.
This try-catch is unnecessary.
| except Exception: | ||
| pass | ||
|
|
||
| return wrapped(*args, **kwargs) |
There was a problem hiding this comment.
You're just doing a direct pass through with this wrapper. I'd imagine we'd at least want to know the operation name and maybe the expiration time and region name.
|
gosh that makes sense :') |
|
@herin049 Addressed the review comments and pushed an update, hope this looks good now. |
| if not is_instrumentation_enabled(): | ||
| return wrapped(*args, **kwargs) | ||
|
|
||
| tracer = get_tracer(__name__, __version__, self.tracer_provider) |
There was a problem hiding this comment.
We need to make sure that the schema version is set. It should be set to _AwsSdkExtension.tracer_schema_version(). I would personally externalize this into another function _get_default_tracer on the current class.
| operation = kwargs.get("ClientMethod") | ||
| if operation: | ||
| span.set_attribute("aws.operation", operation) |
There was a problem hiding this comment.
nit
| operation = kwargs.get("ClientMethod") | |
| if operation: | |
| span.set_attribute("aws.operation", operation) | |
| if operation := kwargs.get("ClientMethod"): | |
| span.set_attribute("aws.operation", operation) |
| region = getattr(instance, "_region_name", None) | ||
| if region: | ||
| span.set_attribute("aws.region", region) |
There was a problem hiding this comment.
| region = getattr(instance, "_region_name", None) | |
| if region: | |
| span.set_attribute("aws.region", region) | |
| if region := getattr(instance, "_region_name", None): | |
| span.set_attribute("aws.region", region) |
| # Region | ||
| region = getattr(instance, "_region_name", None) | ||
| if region: | ||
| span.set_attribute("aws.region", region) |
There was a problem hiding this comment.
Make sure to use the dedicated Span attributes in semconv, or create dedicated constants for these.
| DBUsername="testuser", | ||
| ) | ||
|
|
||
| self.assertIsNotNone(token) |
There was a problem hiding this comment.
We should add assertions to check that the newly added Span attributes are present in the resulting Spans.
There was a problem hiding this comment.
hello @herin049 , I’ve updated the PR based on your suggestion and reworked the presigned URL spans to use the stable rpc.* semconv attributes with an explicit schema URL.
I also adjusted the tests to reflect the schema handling, and the full botocore test suite is passing now.
Please let me know if this matches what you had in mind or if anything should be tweaked further, happy to update. Thank you :)
There was a problem hiding this comment.
Hey @rite7sh thanks for making these changes. Apologies, I should have been more explicit. We do not want use anything with SpanAttribute.* there are deprecated. I simply meant that we should be using constants from the semconv incubating package instead of directly providing strings wherever possible. Also, we should avoid duplicating our tracer schema version if possible, we already have it defined in _AwsSdkExtension.tracer_schema_version()
There was a problem hiding this comment.
@herin049 its all good, I have updated the PR based on the review.Presigned URL spans now use stable semconv + schema handling, and I added a small follow-up commit to clean up imports/formatting after running ruff.
All botocore tests are passing locally. ping me if something feels off, Thanks for the review.
2 participants
Fixes #4040
Type of change
How Has This Been Tested?
test_presigned_url_is_tracedbotocore.presigned_urlis emitted whenbotocore.signers.RequestSigner.generate_presigned_urlis invoked(e.g. via
rds.generate_db_auth_token)To reproduce: