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 2 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.
| if (normalizedRequest.commandInput?.GlobalSecondaryIndexes) { | ||
| spanAttributes[ATTR_AWS_DYNAMODB_GLOBAL_SECONDARY_INDEXES] = toArray( | ||
| normalizedRequest.commandInput.GlobalSecondaryIndexes, | ||
| ).map((x: Record<string, any>) => JSON.stringify(x)); |
There was a problem hiding this comment.
Undocumented any in DynamoDB maps
Low Severity
New Record<string, any> annotations in DynamoDB index and capacity .map callbacks lack the required comment explaining why stricter typing is not used.
Additional Locations (2)
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.
| if (normalizedRequest.commandInput?.GlobalSecondaryIndexes) { | ||
| spanAttributes[ATTR_AWS_DYNAMODB_GLOBAL_SECONDARY_INDEXES] = toArray( | ||
| normalizedRequest.commandInput.GlobalSecondaryIndexes, | ||
| ).map((x: Record<string, any>) => JSON.stringify(x)); |
There was a problem hiding this comment.
Undocumented any in DynamoDB hooks
Low Severity
New DynamoDB extension code uses Record<string, any> in several .map callbacks when stringifying index and capacity objects. Project SDK guidelines require each new any use to include a comment explaining why safer typing is not practical.
Additional Locations (2)
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.
| if (normalizedRequest.commandInput?.GlobalSecondaryIndexes) { | ||
| spanAttributes[ATTR_AWS_DYNAMODB_GLOBAL_SECONDARY_INDEXES] = toArray( | ||
| normalizedRequest.commandInput.GlobalSecondaryIndexes, | ||
| ).map((x: Record<string, any>) => JSON.stringify(x)); |
There was a problem hiding this comment.
Undocumented any in DynamoDB mapper
Low Severity
New DynamoDB extension code uses Record<string, any> in several JSON.stringify map callbacks without comments explaining why stricter typing is not used, which violates SDK source typing guidelines for new any usage.
Additional Locations (2)
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.
| 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
Medium Severity
This feat adds five AWS service span extensions and registry wiring, but the diff includes no unit, integration, or E2E test asserting attributes such as aws.s3.bucket, DynamoDB db.* fields, or Secrets Manager ARNs on orchestrion aws-sdk spans.
Triggered by project rule: PR Review Guidelines for Cursor Bot
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.
| if (normalizedRequest.commandInput?.GlobalSecondaryIndexes) { | ||
| spanAttributes[ATTR_AWS_DYNAMODB_GLOBAL_SECONDARY_INDEXES] = toArray( | ||
| normalizedRequest.commandInput.GlobalSecondaryIndexes, | ||
| ).map((x: Record<string, any>) => JSON.stringify(x)); |
There was a problem hiding this comment.
any types lack justification comments
Low Severity
New Record<string, any> annotations in dynamodb.ts map callbacks have no comments explaining why stricter typing is not used, which violates SDK source typing conventions for this repo.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
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
from
e88a652 to
5285a15
Compare
…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-services
branch
from
5285a15 to
bb30365
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