Skip to content

Commit

Permalink
Merge branch 'bcaudan/opaque' (aa4229b) into staging-51
Browse files Browse the repository at this point in the history
 pm_trace_id: 11825685
 feature_branch_pipeline_id: 11825685
 source: to-staging

* commit 'aa4229b45fa6ebf17bd5f14fdceabd57b0cde2ce':
  🔊add extra debug tag with response type
  🐛don't retry opaque response
  ⚗ fetch keep alive: force 'cors' mode
  • Loading branch information
bcaudan committed Dec 14, 2022
2 parents cd54034 + aa4229b commit 37de1b8
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 41 deletions.
1 change: 1 addition & 0 deletions packages/core/src/domain/configuration/endpointBuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export function createEndpointBuilder(
const tags = [`sdk_version:${__BUILD_ENV__SDK_VERSION__}`, `api:${api}`].concat(configurationTags)
if (retry) {
tags.push(`retry_count:${retry.count}`, `retry_after:${retry.lastFailureStatus}`)
retry.lastFailureType && tags.push(`retry_after_type:${retry.lastFailureType}`)
}
let parameters =
'ddsource=browser' +
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/transport/httpRequest.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,14 @@ describe('httpRequest', () => {
pending('no fetch keepalive support')
}

interceptor.withFetch(() => Promise.resolve({ status: 429 }))
interceptor.withFetch(() => Promise.resolve({ status: 429, type: 'cors' }))

fetchKeepAliveStrategy(
endpointBuilder,
BATCH_BYTES_LIMIT,
{ data: '{"foo":"bar1"}\n{"foo":"bar2"}', bytesCount: 10 },
(response) => {
expect(response).toEqual({ status: 429 })
expect(response).toEqual({ status: 429, type: 'cors' })
done()
}
)
Expand Down
6 changes: 4 additions & 2 deletions packages/core/src/transport/httpRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export type HttpRequest = ReturnType<typeof createHttpRequest>

export interface HttpResponse extends Context {
status: number
type?: ResponseType
}

export interface Payload {
Expand All @@ -29,6 +30,7 @@ export interface Payload {
export interface RetryInfo {
count: number
lastFailureStatus: number
lastFailureType?: ResponseType
}

export function createHttpRequest(
Expand Down Expand Up @@ -91,8 +93,8 @@ export function fetchKeepAliveStrategy(
const canUseKeepAlive = isKeepAliveSupported() && bytesCount < bytesLimit
if (canUseKeepAlive) {
const fetchUrl = endpointBuilder.build('fetch', retry)
fetch(fetchUrl, { method: 'POST', body: data, keepalive: true }).then(
monitor((response: Response) => onResponse?.({ status: response.status })),
fetch(fetchUrl, { method: 'POST', body: data, keepalive: true, mode: 'cors' }).then(
monitor((response: Response) => onResponse?.({ status: response.status, type: response.type })),
monitor(() => {
const xhrUrl = endpointBuilder.build('xhr', retry)
// failed to queue the request
Expand Down
90 changes: 54 additions & 36 deletions packages/core/src/transport/sendWithRetryStrategy.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,50 +190,68 @@ describe('sendWithRetryStrategy', () => {
})
})
;[
{ description: 'when the intake returns error:', status: 500 },
{ description: 'when the intake returns too many request:', status: 429 },
{ description: 'when the intake returns request timeout:', status: 408 },
{ description: 'when network is down:', status: 0 },
].forEach(({ description, status }) => {
{ expectRetry: true, description: 'when the intake returns error:', status: 500 },
{ expectRetry: true, description: 'when the intake returns too many request:', status: 429 },
{ expectRetry: true, description: 'when the intake returns request timeout:', status: 408 },
{ expectRetry: true, description: 'when network error:', status: 0, type: undefined },
{ expectRetry: true, description: 'when network error with response type:', status: 0, type: 'cors' as const },
{ expectRetry: false, description: 'when the intake returns opaque response:', status: 0, type: 'opaque' as const },
].forEach(({ expectRetry, description, status, type }) => {
describe(description, () => {
it('should start queueing following requests', () => {
sendRequest()
sendStub.respondWith(0, { status })
expect(state.queuedPayloads.size()).toBe(1)
if (expectRetry) {
it('should start queueing following requests', () => {
sendRequest()
sendStub.respondWith(0, { status, type })
expect(state.queuedPayloads.size()).toBe(1)

sendRequest()
expect(state.queuedPayloads.size()).toBe(2)
sendRequest()
expect(state.queuedPayloads.size()).toBe(3)
})

sendRequest()
expect(state.queuedPayloads.size()).toBe(2)
sendRequest()
expect(state.queuedPayloads.size()).toBe(3)
})
it('should send queued requests if another ongoing request succeed', () => {
sendRequest()
sendRequest()
sendStub.respondWith(0, { status, type })
expect(state.bandwidthMonitor.ongoingRequestCount).toBe(1)
expect(state.queuedPayloads.size()).toBe(1)

it('should send queued requests if another ongoing request succeed', () => {
sendRequest()
sendRequest()
sendStub.respondWith(0, { status })
expect(state.bandwidthMonitor.ongoingRequestCount).toBe(1)
expect(state.queuedPayloads.size()).toBe(1)
sendRequest()
expect(state.bandwidthMonitor.ongoingRequestCount).toBe(1)
expect(state.queuedPayloads.size()).toBe(2)

sendRequest()
expect(state.bandwidthMonitor.ongoingRequestCount).toBe(1)
expect(state.queuedPayloads.size()).toBe(2)

sendStub.respondWith(1, { status: 200 })
expect(state.bandwidthMonitor.ongoingRequestCount).toBe(2)
expect(state.queuedPayloads.size()).toBe(0)
})
sendStub.respondWith(1, { status: 200 })
expect(state.bandwidthMonitor.ongoingRequestCount).toBe(2)
expect(state.queuedPayloads.size()).toBe(0)
})

it('should add retry info to payloads', () => {
sendRequest()
it('should add retry info to payloads', () => {
sendRequest()

sendStub.respondWith(0, { status })
expect(state.queuedPayloads.first().retry).toEqual({ count: 1, lastFailureStatus: status })
sendStub.respondWith(0, { status, type })
expect(state.queuedPayloads.first().retry).toEqual({
count: 1,
lastFailureStatus: status,
lastFailureType: type,
})

clock.tick(INITIAL_BACKOFF_TIME)
clock.tick(INITIAL_BACKOFF_TIME)

sendStub.respondWith(1, { status })
expect(state.queuedPayloads.first().retry).toEqual({ count: 2, lastFailureStatus: status })
})
sendStub.respondWith(1, { status, type })
expect(state.queuedPayloads.first().retry).toEqual({
count: 2,
lastFailureStatus: status,
lastFailureType: type,
})
})
} else {
it('should not queue the payload for retry', () => {
sendRequest()
sendStub.respondWith(0, { status, type })
expect(state.queuedPayloads.size()).toBe(0)
})
}
})
})

Expand Down
6 changes: 5 additions & 1 deletion packages/core/src/transport/sendWithRetryStrategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ function send(
payload.retry = {
count: payload.retry ? payload.retry.count + 1 : 1,
lastFailureStatus: response.status,
lastFailureType: response.type,
}
onFailure()
}
Expand Down Expand Up @@ -133,7 +134,10 @@ function retryQueuedPayloads(
}

function shouldRetryRequest(response: HttpResponse) {
return response.status === 0 || response.status === 408 || response.status === 429 || response.status >= 500
return (
response?.type !== 'opaque' &&
(response.status === 0 || response.status === 408 || response.status === 429 || response.status >= 500)
)
}

export function newRetryState(): RetryState {
Expand Down

0 comments on commit 37de1b8

Please sign in to comment.