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-854] Enable beforeSend to dismiss events #743

Merged
merged 10 commits into from
Feb 23, 2021
2 changes: 1 addition & 1 deletion packages/core/src/domain/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export type Configuration = typeof DEFAULT_CONFIGURATION &
cookieOptions: CookieOptions

service?: string
beforeSend?: (event: any) => void
beforeSend?: (event: any) => unknown
bcaudan marked this conversation as resolved.
Show resolved Hide resolved

isEnabled: (feature: string) => boolean
}
Expand Down
39 changes: 27 additions & 12 deletions packages/core/src/tools/limitModification.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ describe('limitModification', () => {
candidate.qux = 'modified2'
}

const result = limitModification(object, ['foo.bar', 'qux'], modifier)
limitModification(object, ['foo.bar', 'qux'], modifier)

expect(result).toEqual({
expect(object).toEqual({
foo: { bar: 'modified1' },
qux: 'modified2',
})
Expand All @@ -23,9 +23,9 @@ describe('limitModification', () => {
candidate.qux = 'modified2'
}

const result = limitModification(object, ['foo.bar'], modifier)
limitModification(object, ['foo.bar'], modifier)

expect(result).toEqual({
expect(object).toEqual({
foo: { bar: 'modified1' },
qux: 'qux',
})
Expand All @@ -39,9 +39,9 @@ describe('limitModification', () => {
candidate.qix = 'modified3'
}

const result = limitModification(object, ['foo.bar', 'qux', 'qix'], modifier)
limitModification(object, ['foo.bar', 'qux', 'qix'], modifier)

expect(result).toEqual({
expect(object).toEqual({
foo: { bar: 'modified1' },
qux: 'modified2',
})
Expand All @@ -54,9 +54,9 @@ describe('limitModification', () => {
candidate.qux = 1234
}

const result = limitModification(object, ['foo.bar', 'qux'], modifier)
limitModification(object, ['foo.bar', 'qux'], modifier)

expect(result).toEqual({
expect(object).toEqual({
foo: { bar: 'bar' },
qux: 'qux',
})
Expand All @@ -70,14 +70,29 @@ describe('limitModification', () => {
delete candidate.qux
}

const result = limitModification(object, ['foo.bar', 'qux'], modifier)
limitModification(object, ['foo.bar', 'qux'], modifier)

expect(result).toEqual({
expect(object).toEqual({
foo: { bar: 'bar' },
qux: 'qux',
})
})

it('should return the result of the modifier', () => {
const object = { foo: { bar: 'bar' } }
const modifier = (candidate: any) => {
candidate.foo.bar = 'qux'
return false
}

const result = limitModification(object, ['foo.bar', 'qux'], modifier)

expect(result).toBe(false)
expect(object).toEqual({
foo: { bar: 'qux' },
})
})

it('should catch and log modifier exception', () => {
const object = { foo: { bar: 'bar' }, qux: 'qux' }
const modifier = (candidate: any) => {
Expand All @@ -86,10 +101,10 @@ describe('limitModification', () => {
}
const errorSpy = spyOn(console, 'error')

const result = limitModification(object, ['foo.bar', 'qux'], modifier)
limitModification(object, ['foo.bar', 'qux'], modifier)

expect(errorSpy).toHaveBeenCalled()
expect(result).toEqual({
expect(object).toEqual({
foo: { bar: 'bar' },
qux: 'qux',
})
Expand Down
13 changes: 7 additions & 6 deletions packages/core/src/tools/limitModification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,18 @@ import { Context, deepClone } from './context'
* - field path do not support array, 'a.b.c' only
* - modifiable fields type must be string
*/
export function limitModification<T extends Context>(
export function limitModification<T extends Context, Result>(
object: T,
modifiableFieldPaths: string[],
modifier: (object: T) => void
): T {
modifier: (object: T) => Result
): Result | undefined {
const clone = deepClone(object)
let result
try {
modifier(clone)
result = modifier(clone)
} catch (e) {
console.error(e)
return object
return
}
modifiableFieldPaths.forEach((path) => {
const originalValue = get(object, path)
Expand All @@ -24,7 +25,7 @@ export function limitModification<T extends Context>(
set(object, path, newValue)
}
})
return object
return result
}

function get(object: unknown, path: string) {
Expand Down
15 changes: 12 additions & 3 deletions packages/logs/src/boot/logs.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ describe('logs', () => {

describe('assemble', () => {
let assemble: (message: LogsMessage, currentContext: Context) => Context | undefined
let beforeSend: (event: LogsEvent) => void
let beforeSend: (event: LogsEvent) => void | boolean

beforeEach(() => {
beforeSend = noop
Expand All @@ -148,6 +148,11 @@ describe('logs', () => {
expect(assemble(DEFAULT_MESSAGE, { foo: 'from-current-context' })).toBeUndefined()
})

it('should not assemble if beforeSend returned false', () => {
beforeSend = () => false
expect(assemble(DEFAULT_MESSAGE, { foo: 'from-current-context' })).toBeUndefined()
})

it('add default, current and RUM context to message', () => {
spyOn(window.DD_RUM!, 'getInternalContext').and.returnValue({
view: { url: 'http://from-rum-context.com', id: 'view-id' },
Expand Down Expand Up @@ -188,15 +193,19 @@ describe('logs', () => {
})

it('should allow modification on sensitive field', () => {
beforeSend = (event: LogsEvent) => (event.message = 'modified')
beforeSend = (event: LogsEvent) => {
event.message = 'modified'
}

const assembledMessage = assemble(DEFAULT_MESSAGE, {})

expect(assembledMessage!.message).toBe('modified')
})

it('should reject modification on non sensitive field', () => {
beforeSend = (event: LogsEvent) => ((event.service as any) = 'modified')
beforeSend = (event: LogsEvent) => {
;(event.service as any) = 'modified'
}

const assembledMessage = assemble(DEFAULT_MESSAGE, {})

Expand Down
7 changes: 5 additions & 2 deletions packages/logs/src/boot/logs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { buildEnv } from './buildEnv'

export interface LogsUserConfiguration extends UserConfiguration {
forwardErrorsToLogs?: boolean
beforeSend?: (event: LogsEvent) => void
beforeSend?: (event: LogsEvent) => void | boolean
}

const FIELDS_WITH_SENSITIVE_DATA = ['view.url', 'view.referrer', 'message', 'error.stack', 'http.url']
Expand Down Expand Up @@ -130,11 +130,14 @@ export function buildAssemble(session: LoggerSession, configuration: Configurati
message
)
if (configuration.beforeSend) {
limitModification(
const shouldSend = limitModification(
contextualizedMessage as LogsEvent & Context,
FIELDS_WITH_SENSITIVE_DATA,
configuration.beforeSend
)
if (shouldSend === false) {
return undefined
}
}
return contextualizedMessage as Context
}
Expand Down
2 changes: 1 addition & 1 deletion packages/rum-core/src/boot/rumPublicApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { startRum } from './rum'

export interface RumUserConfiguration extends UserConfiguration {
applicationId: string
beforeSend?: (event: RumEvent) => void
beforeSend?: (event: RumEvent) => void | boolean
}

export type RumPublicApi = ReturnType<typeof makeRumPublicApi>
Expand Down
50 changes: 50 additions & 0 deletions packages/rum-core/src/domain/assembly.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,56 @@ describe('rum assembly', () => {

expect(serverRumEvents[0].view.id).toBe('aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee')
})

it('should allow dismissing events other than views', () => {
beforeSend = () => false

lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, {
rawRumEvent: createRawRumEvent(RumEventType.ACTION, {
view: { id: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' },
}),
startTime: 0,
})

lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, {
rawRumEvent: createRawRumEvent(RumEventType.ERROR, {
view: { id: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' },
}),
startTime: 0,
})

lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, {
rawRumEvent: createRawRumEvent(RumEventType.LONG_TASK, {
view: { id: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' },
}),
startTime: 0,
})

lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, {
rawRumEvent: createRawRumEvent(RumEventType.RESOURCE, {
view: { id: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' },
}),
startTime: 0,
})

expect(serverRumEvents.length).toBe(0)
})

it('should not allow dismissing view events', () => {
beforeSend = () => false
const consoleWarnSpy = spyOn(console, 'warn')
lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, {
rawRumEvent: createRawRumEvent(RumEventType.VIEW, {
view: { id: 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee' },
}),
startTime: 0,
})

expect(serverRumEvents[0].view.id).toBe('aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee')
expect(consoleWarnSpy).toHaveBeenCalledWith(
`Can't dismiss view events using beforeSend, use onNewLocation instead!`
)
})
})

describe('rum context', () => {
Expand Down
19 changes: 15 additions & 4 deletions packages/rum-core/src/domain/assembly.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,27 @@ export function startRumAssembly(
// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
;(serverRumEvent.usr as RumEvent['usr']) = commonContext.user as User & Context
}

if (configuration.beforeSend) {
limitModification(serverRumEvent, FIELDS_WITH_SENSITIVE_DATA, configuration.beforeSend)
if (shouldSend(serverRumEvent, configuration.beforeSend)) {
lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, serverRumEvent)
}
lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, serverRumEvent)
}
}
)
}

function shouldSend(event: RumEvent & Context, beforeSend?: (event: any) => unknown) {
if (beforeSend) {
const result = limitModification(event, FIELDS_WITH_SENSITIVE_DATA, beforeSend)
if (result === false && event.type !== RumEventType.VIEW) {
return false
}
if (result === false) {
console.warn(`Can't dismiss view events using beforeSend, use onNewLocation instead!`)
}
}
return true
}

function needToAssembleWithAction(
event: RawRumEvent
): event is RawRumErrorEvent | RawRumResourceEvent | RawRumLongTaskEvent {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { DOM_EVENT } from '@datadog/browser-core'
import { createRawRumEvent } from '../../../../test/fixtures'
import { Context, DOM_EVENT } from '@datadog/browser-core'
import { RumEvent } from '../../../../../rum/src'
import { setup, TestSetupBuilder } from '../../../../test/specHelper'
import { RumEventType, ActionType } from '../../../rawRumEvent.types'
import { LifeCycle, LifeCycleEventType } from '../../lifeCycle'
Expand Down Expand Up @@ -174,21 +174,18 @@ describe('newAction', () => {

it('counts errors occurring during the action', () => {
const { lifeCycle, clock } = setupBuilder.build()
const collectedRawRumEvent = {
rawRumEvent: createRawRumEvent(RumEventType.ERROR),
startTime: 0,
}
const collectedRumEvent = { type: RumEventType.ERROR } as RumEvent & Context
lifeCycle.subscribe(LifeCycleEventType.AUTO_ACTION_COMPLETED, pushEvent)

newClick('test-1')

lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, collectedRawRumEvent)
lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, collectedRumEvent)
clock.tick(BEFORE_PAGE_ACTIVITY_VALIDATION_DELAY)
lifeCycle.notify(LifeCycleEventType.DOM_MUTATED)
lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, collectedRawRumEvent)
lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, collectedRumEvent)

clock.tick(EXPIRE_DELAY)
lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, collectedRawRumEvent)
lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, collectedRumEvent)

expect(events.length).toBe(1)
const action = events[0]
Expand Down
Loading