feat(server-utils): Port SQS, SNS and Lambda aws-sdk extensions with trace propagation#22165
feat(server-utils): Port SQS, SNS and Lambda aws-sdk extensions with trace propagation#22165andreiborza wants to merge 1 commit into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| debug.warn( | ||
| '[orchestrion:aws-sdk] cannot set trace propagation on SQS/SNS message due to maximum amount of MessageAttributes', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Message attribute quota over-counts
Medium Severity
injectPropagationContext treats every propagation header as a new attribute when checking the 10-attribute SQS/SNS limit, even when sentry-trace or baggage keys already exist. At nine or ten attributes including those keys, injection is skipped even though overwriting would stay within the quota and trace propagation should still run.
Reviewed by Cursor Bugbot for commit 5069f36. Configure here.
|
|
||
| if (Object.keys(attributes).length + headerKeys.length <= MAX_MESSAGE_ATTRIBUTES) { | ||
| for (const key of headerKeys) { | ||
| (attributes as AwsSdkContextObject)[key] = { DataType: 'String', StringValue: headers[key] } as any; |
There was a problem hiding this comment.
Unchecked any cast in injection
Low Severity
New SDK code assigns message attributes with as any and no comment explaining why a narrower type is not possible, which violates the project’s typing guidelines for any in SDK source.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 5069f36. Configure here.
| ); | ||
| } | ||
| return attributes; | ||
| } |
There was a problem hiding this comment.
Feat PR lacks propagation tests
Medium Severity
This feature adds SQS, SNS, and Lambda trace propagation but the diff includes no unit, integration, or E2E test covering injection, extraction, or quota behavior. Testing conventions for feat PRs expect at least one such test.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 5069f36. Configure here.
|
|
||
| function extractQueueUrl(commandInput: Record<string, any>): string { | ||
| return commandInput?.QueueUrl; | ||
| } |
There was a problem hiding this comment.
New any in queue URL helper
Low Severity
extractQueueUrl takes Record<string, any> without a comment explaining the any, which conflicts with SDK guidance on new any usage in production source.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 5069f36. Configure here.
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
03cef18 to
0231bab
Compare
andreiborza
force-pushed
the
ab/aws-sdk-messaging
branch
from
5069f36 to
6abbb22
Compare
| this._services.set('SQS', new SqsServiceExtension()); | ||
| this._services.set('SNS', new SnsServiceExtension()); | ||
| this._services.set('DynamoDB', new DynamodbServiceExtension()); | ||
| this._services.set('Lambda', new LambdaServiceExtension()); |
There was a problem hiding this comment.
Missing tests for new extensions
Medium Severity
This feat adds SQS, SNS, and Lambda trace propagation and span attributes, but the diff includes no unit, integration, or E2E coverage. Per review guidelines, feat PRs should include at least one test that asserts propagation headers, span kinds/names, or receive-side span links.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 6abbb22. Configure here.
|
|
||
| request.commandInput.MessageAttributeNames = addPropagationFieldsToAttributeNames( | ||
| request.commandInput.MessageAttributeNames, | ||
| ); |
There was a problem hiding this comment.
All breaks receive attribute names
Medium Severity
For ReceiveMessage, when MessageAttributeNames is ['All'], addPropagationFieldsToAttributeNames appends sentry-trace and baggage, producing a list that mixes All with specific names. SQS does not allow that combination, so the call can fail or omit propagation headers and span links.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6abbb22. Configure here.
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
0231bab to
c3ac3ce
Compare
andreiborza
force-pushed
the
ab/aws-sdk-messaging
branch
from
6abbb22 to
1964abf
Compare
| if (segments.length === 0) return undefined; | ||
|
|
||
| return segments[segments.length - 1]; | ||
| } |
There was a problem hiding this comment.
Feat PR lacks integration tests
Medium Severity
This feature adds SQS, SNS, and Lambda trace propagation and span behavior, but the diff includes no integration or E2E test covering those paths. Per SDK testing conventions for feat PRs, at least one such test is expected.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 1964abf. Configure here.
andreiborza
force-pushed
the
ab/aws-sdk-messaging
branch
from
1964abf to
f024586
Compare
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
c3ac3ce to
0e20b44
Compare
| if (baggage) { | ||
| headers[BAGGAGE_HEADER] = baggage; | ||
| } | ||
| return headers; |
There was a problem hiding this comment.
Trace propagation skips getTraceData
Medium Severity
getPropagationHeaders builds sentry-trace/baggage via spanToTraceHeader and DSC helpers instead of getTraceData({ span }). Outgoing HTTP uses getTraceData, which handles tracing-without-performance placeholders and skips invalid trace data, so SQS/SNS/Lambda propagation can disagree with other SDK propagation for the same span.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f024586. Configure here.
size-limit report 📦
|
andreiborza
force-pushed
the
ab/aws-sdk-messaging
branch
from
f024586 to
430affd
Compare
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
0e20b44 to
25cd03b
Compare
andreiborza
force-pushed
the
ab/aws-sdk-messaging
branch
from
430affd to
d26e97f
Compare
| export function addPropagationFieldsToAttributeNames(messageAttributeNames: string[] = []): string[] { | ||
| return messageAttributeNames.length | ||
| ? Array.from(new Set([...messageAttributeNames, ...PROPAGATION_FIELDS])) | ||
| : [...PROPAGATION_FIELDS]; |
There was a problem hiding this comment.
All breaks ReceiveMessage mutation
High Severity
addPropagationFieldsToAttributeNames always appends sentry-trace and baggage to MessageAttributeNames on ReceiveMessage. When callers pass All (a common way to fetch every message attribute), AWS SQS rejects requests that combine All with other names, so instrumented receives can fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d26e97f. Configure here.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
078c598 to
979ee5c
Compare
andreiborza
force-pushed
the
ab/aws-sdk-messaging
branch
from
d26e97f to
711b9bd
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 10 total unresolved issues (including 9 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 711b9bd. Configure here.
| }; | ||
| if (request.region) { | ||
| spanAttributes[ATTR_FAAS_INVOKED_REGION] = request.region; | ||
| } |
There was a problem hiding this comment.
Lambda region attribute never set
Medium Severity
faas.invoked_region is only added when request.region is set in requestPreSpanHook, but the orchestrion subscriber always builds the normalized request with region undefined and backfills cloud.region later on the span. Lambda Invoke spans therefore omit faas.invoked_region despite OTel parity expecting it.
Reviewed by Cursor Bugbot for commit 711b9bd. Configure here.
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
979ee5c to
e88a652
Compare
andreiborza
force-pushed
the
ab/aws-sdk-messaging
branch
2 times, most recently
from
3f1cf4a to
c43b14c
Compare
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
e88a652 to
5285a15
Compare
…trace propagation Ports the messaging service extensions from the OTel aws-sdk integration to the orchestrion channel integration and registers them in the service registry: - SQS: producer/consumer span kinds and names, `messaging.*` attributes, trace propagation into outgoing `MessageAttributes` (single and batch), and span links from received messages via the propagated headers - SNS: producer span kind/name for `Publish`, `messaging.*` and topic ARN attributes, trace propagation into `MessageAttributes` - Lambda: `faas.*` attributes for `Invoke`, trace propagation into the base64 `ClientContext` (respecting the 3583 byte cap) Propagation writes Sentry-native `sentry-trace`/`baggage` headers derived from the request span instead of OTel `propagation.inject`, and the SQS receive side reads them back with `propagationContextFromHeaders`. Part of #20946 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
andreiborza
force-pushed
the
ab/aws-sdk-messaging
branch
from
c43b14c to
d993b9e
Compare
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
5285a15 to
bb30365
Compare
1 participant
Ports the messaging service extensions from the OTel aws-sdk integration to the orchestrion channel integration and registers them in the service registry:
messaging.*attributes, trace propagation into outgoingMessageAttributes(single and batch), and span links from received messages via the propagated headersPublish,messaging.*and topic ARN attributes, trace propagation intoMessageAttributesfaas.*attributes forInvoke, trace propagation into the base64ClientContext(respecting the 3583 byte cap)Propagation writes Sentry-native
sentry-trace/baggageheaders derived from the request span instead of OTelpropagation.inject, and the SQS receive side reads them back withpropagationContextFromHeaders.Part of #20946