fix: Avoid overwriting existing trace header #449
Conversation
|
Question: why does Xhr not need to test for the following required for Fetch?
|
| }); | ||
| test('when fetch is called and request has existing trace header then existing trace data is added to the http event', async () => { | ||
| // Init | ||
| console.log(navigator.userAgent); |
There was a problem hiding this comment.
| console.log(navigator.userAgent); |
|
nit: "Use the imperative mood in the subject line" E.g., |
limhjgrace
force-pushed
the
main
branch
2 times, most recently
from
297618e
to
111a886
Compare
limhjgrace
changed the title
For XHR requests, we're unable to access/read the request headers and thus, can't detect if there is an existing trace header. This results in the changes for the XHR plugin to be a subset of the changes for the Fetch plugin and so, some of the tests added for the Fetch plugin don't apply for the XHR plugin changes. |
src/plugins/utils/http-utils.ts
Outdated
| export const isSyntheticsUA = () => { | ||
| return navigator.userAgent.includes('CloudWatchSynthetics'); | ||
| }; |
There was a problem hiding this comment.
nit: Slightly better might be to set a flag inside the constructor of XhrPlugin, so we don't text match against the user agent each time.
| traceId, | ||
| segmentId |
There was a problem hiding this comment.
question: Should we create a new segment?
I think we only need to send a trace segment if we create a new one. Otherwise, we can assume that whoever set the trace header will also send the trace segment to X-Ray.
- Don't create a new segment. Use the trace Id and segment Id from the trace header to set trace_id and segment_id in htttp-event.
- Create a new segment. The new segment will be the child of the segment in the trace header. Change the segment Id in the trace header to be the Id of the new segment.
There was a problem hiding this comment.
I agree that we shouldn't create a new segment. Updated accordingly.
| const headers = (input as Request).headers; | ||
| if (headers) { | ||
| const headerComponents = headers.get(X_AMZN_TRACE_ID)?.split(';'); | ||
| if (headerComponents?.length === 3) { | ||
| traceId = headerComponents[0].split('Root=')[1]; | ||
| segmentId = headerComponents[1].split('Parent=')[1]; | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Stick this in a helper function, and add unit tests. http-utils might be a good place.
| @@ -272,7 +289,11 @@ export class FetchPlugin extends MonkeyPatched<Window, 'fetch'> { | |||
| return original.apply(thisArg, argsArray as any); | |||
| } | |||
|
|
|||
| if (this.isTracingEnabled() && this.isSessionRecorded()) { | |||
| if ( | |||
| !isSyntheticsUA() && | |||
There was a problem hiding this comment.
suggestion: Look for an existing trace header before we reach this point. If there is a header, then simply set httpEvent.trace_id and httpEvent.segment_id. If there is no header, then beginTrace(...)
I don't think we need to use isSyntheticsUA here, since we know whether or not there is a trace header.
When a CW Synthetics canary has
ActiveTracingenabled and generates activity on a web application monitored by a CW RUM app monitor that also hasActiveTracingenabled, users experience a broken X-Ray trace experience where the segment from the canary to the web application is disconnected from the downstream segments. This is because the RUM Web Client overwrites the X-Ray trace header in the HTTP requests with a new trace while the RUM DataPlane drops activity generated by CW Synthetics canaries (and thus the trace is never created/sent to X-Ray for ingestion).This PR will update the Web Client tracing behavior to (1) not trace HTTP requests if the activity is generated by a CW Synthetics canary and (2) not overwrite existing trace headers.
Unit tests should be enough to cover this change, so no integrations were written.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.