botocore: trace presigned URL generation#4096
botocore: trace presigned URL generation#4096rite7sh wants to merge 6 commits intoopen-telemetry:mainfrom
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. |
...pentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/__init__.py
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/__init__.py
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/__init__.py
Outdated
Show resolved
Hide resolved
...pentelemetry-instrumentation-botocore/src/opentelemetry/instrumentation/botocore/__init__.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-botocore/tests/test_botocore_instrumentation.py
Show resolved
Hide resolved
| RPC_SYSTEM: "aws-api", | ||
| RPC_SERVICE: call_context.service_id, | ||
| RPC_METHOD: call_context.operation, | ||
| rpc_attributes.RPC_SYSTEM: "aws-api", |
There was a problem hiding this comment.
Why are we touching this section of code at all in this PR, can we just revert these changes?
| with tracer.start_as_current_span("botocore.presigned_url") as span: | ||
| if service := getattr(instance, "_service_id", None): | ||
| service = service.lower() | ||
| span.set_attribute(rpc_attributes.RPC_SERVICE, service) |
There was a problem hiding this comment.
nit: remove rpc_attributes. prefix
| set_global_textmap(previous_propagator) | ||
|
|
||
| @mock_aws | ||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
Why are we adding this, the tests seemed to have been working fine before this change.
| SERVER_ADDRESS, | ||
| SERVER_PORT, | ||
| ) | ||
| from opentelemetry.semconv.trace import SpanAttributes |
There was a problem hiding this comment.
We need to make sure we don't reintroduce these deprecated attributes even in unit tests.
There was a problem hiding this comment.
@herin049 Thanks for the review I’ve reverted the unrelated changes, tightened the scope to just presigned URL tracing, and all tests pass locally now. Let me know if anything else needs tweaking , my bad for 'extra changes'.
let me know if anything else seems broken :).
rite7sh
force-pushed
the
botocore-presigned-url
branch
from
83dfc96 to
6f45c4e
Compare
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: