Skip to content

Commit

Permalink
🐛[RUMF-617] fix missing headers on traced requests (#517)
Browse files Browse the repository at this point in the history 
* 🐛[RUMF-617] fix missing headers on traced requests

some requests headers possible types were not copied with the tracing headers, resulting in some request failures due to missing headers.

* 🐛 handle correctly multiple values in headers array

* ♻️ use headers array to increase browser support
  • Loading branch information
@bcaudan
bcaudan authored Sep 4, 2020
1 parent f3e14e1 commit a6dc27a
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 46 deletions.
18 changes: 12 additions & 6 deletions packages/rum/src/tracer.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Configuration, FetchContext, getOrigin, XhrContext } from '@datadog/browser-core'
import { Configuration, FetchContext, getOrigin, objectEntries, XhrContext } from '@datadog/browser-core'

export interface TracingResult {
spanId: TraceIdentifier
Expand All @@ -19,15 +19,21 @@ export function startTracer(configuration: Configuration): Tracer {
traceFetch: (context) =>
injectHeadersIfTracingAllowed(configuration, context.url!, (tracingHeaders: TracingHeaders) => {
context.init = { ...context.init }
const headers: { [key: string]: string } = {}

const headers: string[][] = []
if (context.init.headers instanceof Headers) {
context.init.headers.forEach((value, key) => {
headers[key] = value
headers.push([key, value])
})
} else if (Array.isArray(context.init.headers)) {
context.init.headers.forEach((header) => {
headers.push(header)
})
} else if (context.init.headers) {
Object.keys(context.init.headers).forEach((key) => {
headers.push([key, (context.init!.headers as Record<string, string>)[key]])
})
}

context.init.headers = { ...headers, ...tracingHeaders }
context.init.headers = headers.concat(objectEntries(tracingHeaders) as string[][])
}),
traceXhr: (context, xhr) =>
injectHeadersIfTracingAllowed(configuration, context.url!, (tracingHeaders: TracingHeaders) => {
Expand Down
96 changes: 82 additions & 14 deletions packages/rum/test/tracer.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import { Configuration, DEFAULT_CONFIGURATION, FetchContext, isIE, XhrContext } from '@datadog/browser-core'
import {
Configuration,
DEFAULT_CONFIGURATION,
FetchContext,
isIE,
objectEntries,
XhrContext,
} from '@datadog/browser-core'
import { startTracer, TraceIdentifier } from '../src/tracer'
import { setup, TestSetupBuilder } from './specHelper'

Expand Down Expand Up @@ -80,14 +87,11 @@ describe('tracer', () => {
const tracingResult = tracer.traceFetch(context)!

expect(tracingResult).toBeDefined()
expect(context.init!.headers).toEqual(tracingHeadersFor(tracingResult.traceId, tracingResult.spanId))
expect(context.init!.headers).toEqual(tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId))
})

it('should preserve original request init and headers', () => {
const headers = new Headers()
headers.set('foo', 'bar')

const init = { headers, method: 'POST' }
it('should preserve original request init', () => {
const init = { method: 'POST' }
const context: Partial<FetchContext> = {
...ALLOWED_DOMAIN_CONTEXT,
init,
Expand All @@ -98,22 +102,74 @@ describe('tracer', () => {

expect(context.init).not.toBe(init)
expect(context.init!.method).toBe('POST')
expect(context.init!.headers).toEqual(tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId))
})

it('should preserve original headers object', () => {
const headers = new Headers()
headers.set('foo', 'bar')

const context: Partial<FetchContext> = {
...ALLOWED_DOMAIN_CONTEXT,
init: { headers, method: 'POST' },
}

const tracer = startTracer(configuration as Configuration)
const tracingResult = tracer.traceFetch(context)!

expect(context.init!.headers).not.toBe(headers)
expect(context.init!.headers).toEqual({
...tracingHeadersFor(tracingResult.traceId, tracingResult.spanId),
expect(context.init!.headers).toEqual([
['foo', 'bar'],
...tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId),
])
expect(toPlainObject(headers)).toEqual({
foo: 'bar',
})
})

const originalHeadersPlainObject: { [key: string]: string } = {}
headers.forEach((value, key) => {
originalHeadersPlainObject[key] = value
})
expect(originalHeadersPlainObject).toEqual({
it('should preserve original headers plain object', () => {
const headers = { foo: 'bar' }

const context: Partial<FetchContext> = {
...ALLOWED_DOMAIN_CONTEXT,
init: { headers, method: 'POST' },
}

const tracer = startTracer(configuration as Configuration)
const tracingResult = tracer.traceFetch(context)!

expect(context.init!.headers).not.toBe(headers)
expect(context.init!.headers).toEqual([
['foo', 'bar'],
...tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId),
])

expect(headers).toEqual({
foo: 'bar',
})
})

it('should preserve original headers array', () => {
const headers = [['foo', 'bar'], ['foo', 'baz']]

const context: Partial<FetchContext> = {
...ALLOWED_DOMAIN_CONTEXT,
init: { headers, method: 'POST' },
}

const tracer = startTracer(configuration as Configuration)
const tracingResult = tracer.traceFetch(context)!

expect(context.init!.headers).not.toBe(headers)
expect(context.init!.headers).toEqual([
['foo', 'bar'],
['foo', 'baz'],
...tracingHeadersAsArrayFor(tracingResult.traceId, tracingResult.spanId),
])

expect(headers).toEqual([['foo', 'bar'], ['foo', 'baz']])
})

it('should not trace request on disallowed domain', () => {
const context: Partial<FetchContext> = { ...DISALLOWED_DOMAIN_CONTEXT }

Expand Down Expand Up @@ -148,6 +204,14 @@ describe('TraceIdentifier', () => {
})
})

function toPlainObject(headers: Headers) {
const result: { [key: string]: string } = {}
headers.forEach((value, key) => {
result[key] = value
})
return result
}

function tracingHeadersFor(traceId: TraceIdentifier, spanId: TraceIdentifier) {
return {
'x-datadog-origin': 'rum',
Expand All @@ -157,3 +221,7 @@ function tracingHeadersFor(traceId: TraceIdentifier, spanId: TraceIdentifier) {
'x-datadog-trace-id': traceId.toDecimalString(),
}
}

function tracingHeadersAsArrayFor(traceId: TraceIdentifier, spanId: TraceIdentifier) {
return objectEntries(tracingHeadersFor(traceId, spanId)) as string[][]
}
5 changes: 3 additions & 2 deletions test/e2e/scenario/agents.scenario.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,14 +308,14 @@ describe('anchor navigation', () => {

describe('tracing', () => {
it('should trace xhr', async () => {
const rawHeaders = await sendXhr(`${serverUrl.sameOrigin}/headers`)
const rawHeaders = await sendXhr(`${serverUrl.sameOrigin}/headers`, [['x-foo', 'bar'], ['x-foo', 'baz']])
checkRequestHeaders(rawHeaders)
await flushEvents()
await checkTraceAssociatedToRumEvent()
})

it('should trace fetch', async () => {
const rawHeaders = await sendFetch(`${serverUrl.sameOrigin}/headers`)
const rawHeaders = await sendFetch(`${serverUrl.sameOrigin}/headers`, [['x-foo', 'bar'], ['x-foo', 'baz']])
checkRequestHeaders(rawHeaders)
await flushEvents()
await checkTraceAssociatedToRumEvent()
Expand All @@ -325,6 +325,7 @@ describe('tracing', () => {
const headers: { [key: string]: string } = JSON.parse(rawHeaders) as any
expect(headers['x-datadog-trace-id']).toMatch(/\d+/)
expect(headers['x-datadog-origin']).toBe('rum')
expect(headers['x-foo']).toBe('bar, baz')
}

async function checkTraceAssociatedToRumEvent() {
Expand Down
57 changes: 33 additions & 24 deletions test/e2e/scenario/helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,32 +171,41 @@ export async function makeXHRAndCollectEvent(url: string): Promise<ServerRumReso
return (await waitServerRumEvents()).filter(isRumResourceEvent).find((event) => event.http.url === url)
}

export async function sendXhr(url: string): Promise<string> {
// tslint:disable-next-line: no-shadowed-variable
return browserExecuteAsync((url, done) => {
let loaded = false
const xhr = new XMLHttpRequest()
xhr.addEventListener('load', () => (loaded = true))
xhr.open('GET', url)
xhr.send()

const interval = setInterval(() => {
if (loaded) {
clearInterval(interval)
done(xhr.response as string)
}
}, 500)
}, url)
export async function sendXhr(url: string, headers: string[][] = []): Promise<string> {
return browserExecuteAsync(
// tslint:disable-next-line: no-shadowed-variable
(url, headers, done) => {
let loaded = false
const xhr = new XMLHttpRequest()
xhr.addEventListener('load', () => (loaded = true))
xhr.open('GET', url)
headers.forEach((header) => xhr.setRequestHeader(header[0], header[1]))
xhr.send()

const interval = setInterval(() => {
if (loaded) {
clearInterval(interval)
done(xhr.response as string)
}
}, 500)
},
url,
headers
)
}

export async function sendFetch(url: string): Promise<string> {
// tslint:disable-next-line: no-shadowed-variable
return browserExecuteAsync((url, done) => {
window
.fetch(url)
.then((response) => response.text())
.then(done)
}, url)
export async function sendFetch(url: string, headers: string[][] = []): Promise<string> {
return browserExecuteAsync(
// tslint:disable-next-line: no-shadowed-variable
(url, headers, done) => {
window
.fetch(url, { headers })
.then((response) => response.text())
.then(done)
},
url,
headers
)
}

export function expectToHaveValidTimings(resourceEvent: ServerRumResourceEvent) {
Expand Down

0 comments on commit a6dc27a

Please sign in to comment.