opentelemetry-instrumentation-boto3sqs: migrate to current messaging semantic conventions - #4920
opentelemetry-instrumentation-boto3sqs: migrate to current messaging semantic conventions#4920anneheartrecord wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 778f72b085
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| operation_name: str, | ||
| operation_type: MessagingOperationTypeValues, |
There was a problem hiding this comment.
Removing queue_url from _enrich_span drops the previous endpoint information without replacing it with the current convention's server.address (and server.port for non-default ports), even though every instrumented call already supplies the queue URL. As a result, spans for AWS endpoints and especially LocalStack/custom endpoints can no longer identify the server that handled the operation, and the declared v1.27 messaging telemetry is incomplete; retain and parse the URL using the generated server attribute constants.
AGENTS.md reference: AGENTS.md:L84-L87
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — added server.address (and server.port when the URL carries an explicit port, e.g. LocalStack) parsed from the queue URL in _enrich_span, replacing the endpoint info the old messaging.url attribute provided. Covered by the updated default-attrs assertions plus a new custom-endpoint test. 5804e8c
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5804e8c649
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if parsed_url.port: | ||
| span.set_attribute(SERVER_PORT, parsed_url.port) |
There was a problem hiding this comment.
When a custom QueueUrl contains a nonnumeric or out-of-range port, accessing parsed_url.port raises ValueError before the wrapped botocore operation runs. Previously this value was forwarded so botocore could produce its own validation or request error; the instrumentation now changes application behavior instead of preserving the underlying exception. Skip the server attributes when URL parsing fails.
AGENTS.md reference: AGENTS.md:L80-L82
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, this one was a real behaviour change — urlparse(...).port raises for http://localhost:not-a-port/... (and urlparse itself raises on an unclosed IPv6 bracket), so the instrumentation was failing the call before botocore could report the bad URL. Fixed in 5348926: parsing is wrapped and both server attributes are skipped when it fails, with tests for the malformed-port and unparsable-URL cases.
| if parsed_url.port: | ||
| span.set_attribute(SERVER_PORT, parsed_url.port) |
There was a problem hiding this comment.
When a valid queue URL explicitly includes its scheme's default port, such as https://host:443/... or http://host:80/..., this truthiness check emits server.port. The current server semantic convention requires omitting the attribute for the default port, so the newly added endpoint enrichment produces nonconforming telemetry for these URLs; compare the parsed scheme and only set non-default ports.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I went the other way here. For messaging spans server.port is Recommended with no condition attached to the default port — the omit-if-default requirement is the HTTP client convention, not the server registry one. And the AWS sibling in this repo emits it unconditionally: botocore/utils.py:get_server_attributes sets parsed.port or 443.
What the previous version did get wrong is that the attribute depended on how the caller spelled the endpoint — https://host:443/q got a port and https://host/q did not, for the same server. 5348926 makes it uniform instead: explicit port when present, otherwise the scheme default (443/80), so boto3sqs spans carry the same server.port as the botocore spans for the same call. If you would rather omit defaults here, I am happy to flip it — it is a one-line change either way.
I did not reuse botocore's helper on purpose: opentelemetry-instrumentation-boto3sqs does not depend on the botocore instrumentation package (deps are api, instrumentation, semconv, wrapt), so sharing it would mean adding one.
Reading the port of a malformed queue URL raised before the wrapped botocore call ran, so a bad QueueUrl surfaced as a ValueError from the instrumentation instead of botocore's own error. Fall back to no server attributes in that case, and derive the port from the scheme when the URL omits it so it no longer depends on how the caller spelled the endpoint.
1 participant
Description
Migrates the boto3sqs instrumentation off the deprecated
opentelemetry.semconv.trace.SpanAttributes(schema 1.11.0) onto the current incubating messaging attributes, following the pattern already used by the aiokafka instrumentation:messaging.system:aws.sqs→aws_sqs(MessagingSystemValues.AWS_SQS)messaging.destination→messaging.destination.namemessaging.operation→messaging.operation.name(send/receive/process) +messaging.operation.type; send spans now carry operation attributes toomessaging.destination_kindandmessaging.url(removed from the spec)messaging.conversation_id, which was being set to thesend_message_batchentryId(not a conversation/message-group id per spec)Schemas.V1_27_0(same as aiokafka)Span names are intentionally left as
{queue} {operation}to match the existing aiokafka naming and avoid an extra breaking change; happy to adjust if you'd rather move to{operation} {queue}here as well.Fixes #1639
Type of change
How Has This Been Tested?
tests/test_boto3sqs_instrumentation.pyto assert the new attributes; full suite passes locally (16 passed) with boto3/botocore 1.34.44 (the pinned test versions), plusruff check/ruff format --check.Does This PR Require a Core Repo Change?
Checklist: