feat(server-utils): Port S3, Kinesis, DynamoDB, SecretsManager and StepFunctions aws-sdk extensions#22164
feat(server-utils): Port S3, Kinesis, DynamoDB, SecretsManager and StepFunctions aws-sdk extensions#22164andreiborza wants to merge 5 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (response.data?.ScannedCount) { | ||
| span.setAttribute(ATTR_AWS_DYNAMODB_SCANNED_COUNT, response.data?.ScannedCount); |
There was a problem hiding this comment.
Truthy checks skip DynamoDB attributes
Medium Severity
Several DynamoDB span attributes are only set inside truthy if checks, so valid values like ScanIndexForward: false, parallel Segment: 0, and response Count or ScannedCount of zero never appear on spans even when the API returned them.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 03cef18. Configure here.
| this._services.set('SFN', new StepFunctionsServiceExtension()); | ||
| this._services.set('DynamoDB', new DynamodbServiceExtension()); | ||
| this._services.set('S3', new S3ServiceExtension()); | ||
| this._services.set('Kinesis', new KinesisServiceExtension()); |
There was a problem hiding this comment.
Feat PR lacks integration tests
Low Severity
This feature adds five AWS service span extensions and registers them in ServicesExtensions, but the diff includes no integration or E2E test asserting the new span attributes (similar to existing orchestrion channel tests for Postgres).
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 03cef18. Configure here.
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
3 times, most recently
from
c3ac3ce to
0e20b44
Compare
| this._services.set('SFN', new StepFunctionsServiceExtension()); | ||
| this._services.set('DynamoDB', new DynamodbServiceExtension()); | ||
| this._services.set('S3', new S3ServiceExtension()); | ||
| this._services.set('Kinesis', new KinesisServiceExtension()); |
There was a problem hiding this comment.
Feature lacks integration tests
Low Severity
This feat change registers new S3, Kinesis, DynamoDB, Secrets Manager, and Step Functions span attribute hooks, but the diff adds no unit, integration, or E2E coverage asserting those attributes on spans. Per project testing conventions, a feature PR should include at least one test exercising the new behavior.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 0e20b44. Configure here.
size-limit report 📦
|
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
3 times, most recently
from
078c598 to
979ee5c
Compare
| this._services.set('SFN', new StepFunctionsServiceExtension()); | ||
| this._services.set('DynamoDB', new DynamodbServiceExtension()); | ||
| this._services.set('S3', new S3ServiceExtension()); | ||
| this._services.set('Kinesis', new KinesisServiceExtension()); |
There was a problem hiding this comment.
Missing integration or E2E tests
Medium Severity
This feat PR registers new AWS service span attribute extensions but the diff adds no unit, integration, or E2E test that exercises the orchestrion aws-sdk channel and asserts attributes such as aws.s3.bucket or db.* on emitted spans.
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 979ee5c. Configure here.
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
979ee5c to
e88a652
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
There are 11 total unresolved issues (including 7 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 e88a652. Configure here.
|
|
||
| if (response.data?.ScannedCount) { | ||
| span.setAttribute(ATTR_AWS_DYNAMODB_SCANNED_COUNT, response.data?.ScannedCount); | ||
| } |
There was a problem hiding this comment.
Zero counts omitted from spans
Low Severity
responseHook only sets aws.dynamodb.count and aws.dynamodb.scanned_count when response.data.Count and response.data.ScannedCount are truthy, so legitimate zero values from Query or Scan are never recorded on the span.
Reviewed by Cursor Bugbot for commit e88a652. Configure here.
| if (operation === 'Scan') { | ||
| if (normalizedRequest.commandInput?.Segment) { | ||
| spanAttributes[ATTR_AWS_DYNAMODB_SEGMENT] = normalizedRequest.commandInput?.Segment; | ||
| } |
There was a problem hiding this comment.
Scan segment zero skipped
Low Severity
For Scan, aws.dynamodb.segment is set only when commandInput.Segment is truthy, so parallel scans using segment 0 never get the segment attribute on the span.
Reviewed by Cursor Bugbot for commit e88a652. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 8 total unresolved issues (including 7 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 e88a652. Configure here.
| span.setAttribute(ATTR_AWS_DYNAMODB_SCANNED_COUNT, response.data?.ScannedCount); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Feature lacks integration tests
Medium Severity
This feature adds several AWS service span attribute extensions but the diff includes no unit, integration, or E2E coverage. Per SDK testing conventions for feat PRs, at least one test should assert the new attributes on spans for services such as DynamoDB or S3.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit e88a652. Configure here.
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
4 times, most recently
from
07afc91 to
36d9255
Compare
andreiborza
force-pushed
the
ab/aws-sdk-server-utils
branch
from
52e07f6 to
69d0289
Compare
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
2 times, most recently
from
c61e735 to
7248300
Compare
andreiborza
force-pushed
the
ab/aws-sdk-server-utils
branch
from
d87099e to
69d0289
Compare
Reverts the makeSafeSpanBuilder extraction (and the graphql change that came with it) in favor of plain try/catch blocks at the three guard sites. The post-span hook and region provider keep their own guards so a throw there cannot discard the already-started span via the outer catch.
…epFunctions aws-sdk extensions Ports the attribute-only service extensions from the OTel aws-sdk integration to the orchestrion channel integration and registers them in the service registry: - S3: `aws.s3.bucket` - Kinesis: `aws.kinesis.stream.name` - DynamoDB: `db.*` and `aws.dynamodb.*` request/response attributes - SecretsManager: `aws.secretsmanager.secret.arn` (request and response) - StepFunctions: state machine / activity ARNs Straight ports; none of these inject trace propagation or change span lifecycle. Part of #20946 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
andreiborza
force-pushed
the
ab/aws-sdk-server-utils
branch
from
69d0289 to
9510b15
Compare
andreiborza
force-pushed
the
ab/aws-sdk-services
branch
from
7248300 to
50ce000
Compare
1 participant
Ports the attribute-only service extensions from the OTel aws-sdk integration to the orchestrion channel integration and registers them in the service registry:
aws.s3.bucketaws.kinesis.stream.namedb.*andaws.dynamodb.*request/response attributesaws.secretsmanager.secret.arn(request and response)Straight ports; none of these inject trace propagation or change span lifecycle.
Part of #20946