Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
"skippedTestReport": "ts-node ./scripts/skippedTestReport.ts ./packages/amazonq/test/e2e/"
},
"devDependencies": {
"@aws-toolkits/telemetry": "^1.0.311",
"@aws-toolkits/telemetry": "^1.0.312",
"@playwright/browser-chromium": "^1.43.1",
"@stylistic/eslint-plugin": "^2.11.0",
"@types/he": "^1.2.3",
Expand Down
26 changes: 20 additions & 6 deletions packages/core/src/codewhisperer/service/securityScanHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -384,20 +384,24 @@ export async function uploadArtifactToS3(
headersObj['x-amz-server-side-encryption-aws-kms-key-id'] = resp.kmsKeyArn
}

let requestId: string | undefined = undefined
let id2: string | undefined = undefined
let responseCode: string = ''

try {
const response = await request.fetch('PUT', resp.uploadUrl, {
body: readFileSync(fileName),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible this exceeds a request size limit? Should it be streamed?

headers: resp?.requestHeaders ?? headersObj,
}).response
logger.debug(`StatusCode: ${response.status}, Text: ${response.statusText}`)
requestId = response.headers?.get('x-amz-request-id') ?? undefined
id2 = response.headers?.get('x-amz-id-2') ?? undefined
responseCode = response.status.toString()
Copy link
Contributor

@justinmk3 justinmk3 Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not great that all of this is placed in the business logic for a feature. This kind of thing should live in shared/request. It is generally useful for many features.

And clearly, it's not easy to get right, since this PR is already fixing a bug related to it. Yet this bug fix won't be shared with other similar code that needs to make requests (even if it's specific to S3 PUT).

} catch (error) {
if (span && error instanceof RequestError) {
const requestId = error.response.headers.get('x-amz-request-id') ?? undefined
span.record({
requestId: requestId,
requestServiceType: 's3',
httpStatusCode: error.code.toString(),
})
requestId = error.response.headers.get('x-amz-request-id') ?? undefined
id2 = error.response.headers.get('x-amz-id-2') ?? undefined
responseCode = error.code.toString()
}
let errorMessage = ''
const isCodeScan = featureUseCase === FeatureUseCase.CODE_SCAN
Expand All @@ -419,6 +423,16 @@ export async function uploadArtifactToS3(
ChatSessionManager.Instance.getSession().startTestGenerationRequestId = error.requestId
}
throw isCodeScan ? new UploadArtifactToS3Error(errorMessage) : new UploadTestArtifactToS3Error(errorMessage)
} finally {
getLogger().debug(`Upload to S3 response details: x-amz-request-id: ${requestId}, x-amz-id-2: ${id2}`)
if (span) {
span.record({
requestId: requestId,
requestId2: id2,
requestServiceType: 's3',
httpStatusCode: responseCode,
})
}
}
}

Expand Down
Loading