-
Notifications
You must be signed in to change notification settings - Fork 736
telemetry(amazonq): Add id2 and logging for createUpload metrics #6857
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
|
||
| try { | ||
| const response = await request.fetch('PUT', resp.uploadUrl, { | ||
| body: readFileSync(fileName), |
There was a problem hiding this comment.
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?
| 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() |
There was a problem hiding this comment.
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).
Problem
Some users are reporting S3 upload failures but we don't have the id2 that S3 team requires to investigate further.
Related Commons change: #1005
Solution
Emit amazonq_createUpload metric with id2, and add logs for these ids so customers can use too.
feature/xbranches will not be squash-merged at release time.