Skip to content
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

🐛 [RUMF-1303] stop forwarding network errors when forwardErrorsToLogs is false #1591

Merged
merged 5 commits into from
Jun 15, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ describe('network error collection', () => {
url: FAKE_URL,
}

function startCollection(forwardErrorsToLogs = true) {
fetchStubManager = stubFetch()
;({ stop: stopNetworkErrorCollection } = startNetworkErrorCollection(
{ ...CONFIGURATION, forwardErrorsToLogs },
lifeCycle
))
fetchStub = window.fetch as FetchStub
}

beforeEach(() => {
if (isIE()) {
pending('no fetch support')
Expand All @@ -45,9 +54,6 @@ describe('network error collection', () => {
lifeCycle.subscribe(LifeCycleEventType.RAW_LOG_COLLECTED, (rawLogsEvent) =>
rawLogsEvents.push(rawLogsEvent as RawLogsEventCollectedData<RawNetworkLogsEvent>)
)
fetchStubManager = stubFetch()
;({ stop: stopNetworkErrorCollection } = startNetworkErrorCollection(CONFIGURATION, lifeCycle))
fetchStub = window.fetch as FetchStub
})

afterEach(() => {
Expand All @@ -56,6 +62,7 @@ describe('network error collection', () => {
})

it('should track server error', (done) => {
startCollection()
fetchStub(FAKE_URL).resolveWith(DEFAULT_REQUEST)

fetchStubManager.whenAllComplete(() => {
Expand All @@ -78,7 +85,18 @@ describe('network error collection', () => {
})
})

it('should not track network error when forwardErrorsToLogs is false', (done) => {
startCollection(false)
fetchStub(FAKE_URL).resolveWith(DEFAULT_REQUEST)

fetchStubManager.whenAllComplete(() => {
expect(rawLogsEvents.length).toEqual(0)
done()
})
})

it('should not track intake error', (done) => {
startCollection()
fetchStub('https://logs-intake.com/v1/input/send?foo=bar').resolveWith(DEFAULT_REQUEST)

fetchStubManager.whenAllComplete(() => {
Expand All @@ -88,6 +106,7 @@ describe('network error collection', () => {
})

it('should track aborted requests', (done) => {
startCollection()
fetchStub(FAKE_URL).abort()

fetchStubManager.whenAllComplete(() => {
Expand All @@ -97,6 +116,7 @@ describe('network error collection', () => {
})

it('should track refused request', (done) => {
startCollection()
fetchStub(FAKE_URL).resolveWith({ ...DEFAULT_REQUEST, status: 0 })

fetchStubManager.whenAllComplete(() => {
Expand All @@ -106,6 +126,7 @@ describe('network error collection', () => {
})

it('should not track client error', (done) => {
startCollection()
fetchStub(FAKE_URL).resolveWith({ ...DEFAULT_REQUEST, status: 400 })

fetchStubManager.whenAllComplete(() => {
Expand All @@ -115,6 +136,7 @@ describe('network error collection', () => {
})

it('should not track successful request', (done) => {
startCollection()
fetchStub(FAKE_URL).resolveWith({ ...DEFAULT_REQUEST, status: 200 })

fetchStubManager.whenAllComplete(() => {
Expand All @@ -124,6 +146,7 @@ describe('network error collection', () => {
})

it('uses a fallback when the response text is empty', (done) => {
startCollection()
fetchStub(FAKE_URL).resolveWith({ ...DEFAULT_REQUEST, responseText: '' })

fetchStubManager.whenAllComplete(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ import { LifeCycleEventType } from '../../lifeCycle'
import { StatusType } from '../../logger'

export function startNetworkErrorCollection(configuration: LogsConfiguration, lifeCycle: LifeCycle) {
if (!configuration.forwardErrorsToLogs) {
return { stop: noop }
}

const xhrSubscription = initXhrObservable().subscribe((context) => {
if (context.state === 'complete') {
handleCompleteRequest(RequestType.XHR, context)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { ErrorSource, Observable } from '@datadog/browser-core'
import type { RawError, RelativeTime, TimeStamp } from '@datadog/browser-core'
import { ErrorSource } from '@datadog/browser-core'
import type { RawRuntimeLogsEvent } from '../../../rawLogsEvent.types'
import type { LogsConfiguration } from '../../configuration'
import { StatusType } from '../../logger'
Expand All @@ -8,43 +7,60 @@ import { LifeCycle, LifeCycleEventType } from '../../lifeCycle'
import { startRuntimeErrorCollection } from './runtimeErrorCollection'

describe('runtime error collection', () => {
let rawErrorObservable: Observable<RawError>
const configuration = { forwardErrorsToLogs: true } as LogsConfiguration
let lifeCycle: LifeCycle
let stopRuntimeErrorCollection: () => void
let rawLogsEvents: Array<RawLogsEventCollectedData<RawRuntimeLogsEvent>>
let onErrorSpy: jasmine.Spy
let originalOnErrorHandler: OnErrorEventHandler

beforeEach(() => {
originalOnErrorHandler = window.onerror
onErrorSpy = jasmine.createSpy()
window.onerror = onErrorSpy
rawLogsEvents = []
rawErrorObservable = new Observable<RawError>()
lifeCycle = new LifeCycle()
lifeCycle.subscribe(LifeCycleEventType.RAW_LOG_COLLECTED, (rawLogsEvent) =>
rawLogsEvents.push(rawLogsEvent as RawLogsEventCollectedData<RawRuntimeLogsEvent>)
)
;({ stop: stopRuntimeErrorCollection } = startRuntimeErrorCollection(
{} as LogsConfiguration,
lifeCycle,
rawErrorObservable
))
})

afterEach(() => {
stopRuntimeErrorCollection()
window.onerror = originalOnErrorHandler
})

it('should send runtime errors', () => {
rawErrorObservable.notify({
message: 'error!',
source: ErrorSource.SOURCE,
startClocks: { relative: 1234 as RelativeTime, timeStamp: 123456789 as TimeStamp },
type: 'Error',
it('should send runtime errors', (done) => {
Copy link
Contributor Author

@amortemousque amortemousque Jun 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the test to really trigger a runtime error instead of using the observable, because I found it more secure.
With the previous implem of runtimeErrorCollection, when sending a message in rawErrorObservable the collection worked even if the forwardErrorsToLogs was false.

;({ stop: stopRuntimeErrorCollection } = startRuntimeErrorCollection(configuration, lifeCycle))
setTimeout(() => {
throw new Error('error!')
})

expect(rawLogsEvents[0].rawLogsEvent).toEqual({
date: 123456789 as TimeStamp,
error: { origin: ErrorSource.SOURCE, kind: 'Error', stack: undefined },
message: 'error!',
status: StatusType.error,
origin: ErrorSource.SOURCE,
setTimeout(() => {
expect(rawLogsEvents[0].rawLogsEvent).toEqual({
date: jasmine.any(Number),
error: { origin: ErrorSource.SOURCE, kind: 'Error', stack: jasmine.any(String) },
message: 'error!',
status: StatusType.error,
origin: ErrorSource.SOURCE,
})
done()
}, 10)
})

it('should not send runtime errors when forwardErrorsToLogs is false', (done) => {
;({ stop: stopRuntimeErrorCollection } = startRuntimeErrorCollection(
{ ...configuration, forwardErrorsToLogs: false },
lifeCycle
))

setTimeout(() => {
throw new Error('error!')
})

setTimeout(() => {
expect(rawLogsEvents.length).toEqual(0)
done()
}, 10)
})
})
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { Context, RawError, ClocksState } from '@datadog/browser-core'
import { ErrorSource, trackRuntimeError, Observable } from '@datadog/browser-core'
import { noop, ErrorSource, trackRuntimeError, Observable } from '@datadog/browser-core'
import type { RawRuntimeLogsEvent } from '../../../rawLogsEvent.types'
import type { LogsConfiguration } from '../../configuration'
import type { LifeCycle } from '../../lifeCycle'
Expand All @@ -13,15 +13,15 @@ export interface ProvidedError {
handlingStack: string
}

export function startRuntimeErrorCollection(
configuration: LogsConfiguration,
lifeCycle: LifeCycle,
rawErrorObservable = new Observable<RawError>()
) {
if (configuration.forwardErrorsToLogs) {
trackRuntimeError(rawErrorObservable)
export function startRuntimeErrorCollection(configuration: LogsConfiguration, lifeCycle: LifeCycle) {
if (!configuration.forwardErrorsToLogs) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid to unnecessary listen to the observable when forwardErrorsToLogs is false

return { stop: noop }
}

const rawErrorObservable = new Observable<RawError>()

const { stop: stopRuntimeErrorTracking } = trackRuntimeError(rawErrorObservable)

const rawErrorSubscription = rawErrorObservable.subscribe((rawError) => {
lifeCycle.notify<RawRuntimeLogsEvent>(LifeCycleEventType.RAW_LOG_COLLECTED, {
rawLogsEvent: {
Expand All @@ -40,6 +40,7 @@ export function startRuntimeErrorCollection(

return {
stop: () => {
stopRuntimeErrorTracking()
rawErrorSubscription.unsubscribe()
},
}
Expand Down