-
Notifications
You must be signed in to change notification settings - Fork 63
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
fix: Set appropriate error codes in X-Ray segments #192
Conversation
@@ -143,14 +145,31 @@ export class FetchPlugin extends MonkeyPatched<Window, 'fetch'> { | |||
xRayTraceEvent.subsegments[0].http.response = { | |||
status: response.status | |||
}; | |||
|
|||
if (is429(response.status)) { | |||
xRayTraceEvent.subsegments[0].throttle = true; |
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.
question: I'm assuming subsegments[0]
refers to the very first downstream call from the application. If that call returns an error/fault/throttle, we update subsegments[0]
accordingly. How is subsegment[1..n]
updated?
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.
Looking at the code, it seems like we're responsible for generating the subsegment of the first downstream call, which makes sense.
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.
Correct -- there is only one subsegment.
const cl = parseInt(response.headers.get('Content-Length'), 10); | ||
if (!isNaN(cl)) { | ||
xRayTraceEvent.subsegments[0].http.response.content_length = cl; | ||
} | ||
} | ||
|
||
if (error) { | ||
xRayTraceEvent.subsegments[0].error = true; |
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.
question: are Error
objects only created for 5xx?
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.
If an error is passed in along with a status code of 4xx, it will lead to xRayTraceEvent.subsegments[0].error = true, xRayTraceEvent.subsegments[0].fault=true, xRayTraceEvent.fault = true, xRayTraceEvent.fault = true
. Is it a case we do not have to worry about?
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.
In this branch a network error has occurred. There is no response and by extension no status code.
When an error, fault or throttle event occurs, the web client does not currently set the appropriate status flag in the X-Ray segment or subsegment. When viewing the trace in X-Ray, this causes the request to look like it succeeded when it failed.
According to the X-Ray segment documentation:
This change sets the appropriate
error
,fault
orthrottle
flag when (1) a 4xx or 5xx status code is returned, or (2) the http request fails.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.