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 1470] Fix serialization issues #1971

Merged
merged 12 commits into from
Mar 27, 2023
Merged
2 changes: 1 addition & 1 deletion packages/core/src/domain/console/consoleObservable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ function buildConsoleLog(params: unknown[], api: ConsoleApiName, handlingStack:

function formatConsoleParameters(param: unknown) {
if (typeof param === 'string') {
return param
return sanitize(param)
}
if (param instanceof Error) {
return formatErrorMessage(computeStackTrace(param))
Expand Down
26 changes: 26 additions & 0 deletions packages/core/src/tools/sanitize.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { isIE } from './browserDetection'
import { display } from './display'
import { sanitize } from './sanitize'

describe('sanitize', () => {
Expand Down Expand Up @@ -96,6 +97,25 @@ describe('sanitize', () => {
expect(sanitize(map)).toBe('[Map]')
}
})

it('should survive an improperly implemented toStringTag', () => {
function Func() {
/* empty */
}

Object.defineProperty(Func.prototype, Symbol.toStringTag, {
get: () => {
throw Error('Cannot serialize')
},
enumerable: false,
configurable: true,
})

// eslint-disable-next-line @typescript-eslint/no-unsafe-call
const func = new (Func as any)()
yannickadam marked this conversation as resolved.
Show resolved Hide resolved
yannickadam marked this conversation as resolved.
Show resolved Hide resolved

expect(sanitize(func)).toEqual('[Unserializable]')
})
})

describe('arrays handling', () => {
Expand Down Expand Up @@ -202,21 +222,27 @@ describe('sanitize', () => {

describe('maxSize verification', () => {
it('should return nothing if a simple type is over max size ', () => {
const displaySpy = spyOn(display, 'warn')
const str = 'A not so long string...'

expect(sanitize(str, 5)).toBe(undefined)
expect(displaySpy).toHaveBeenCalled()
})

it('should stop cloning if an object container type reaches max size', () => {
const displaySpy = spyOn(display, 'warn')
const obj = { a: 'abc', b: 'def', c: 'ghi' } // Length of 31 after JSON.stringify
const sanitized = sanitize(obj, 21)
expect(sanitized).toEqual({ a: 'abc', b: 'def' }) // Length of 21 after JSON.stringify
expect(displaySpy).toHaveBeenCalled()
})

it('should stop cloning if an array container type reaches max size', () => {
const displaySpy = spyOn(display, 'warn')
const obj = [1, 2, 3, 4] // Length of 9 after JSON.stringify
const sanitized = sanitize(obj, 5)
expect(sanitized).toEqual([1, 2]) // Length of 5 after JSON.stringify
expect(displaySpy).toHaveBeenCalled()
})
})
})
44 changes: 34 additions & 10 deletions packages/core/src/tools/sanitize.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Context, ContextArray, ContextValue } from './context'
import { display } from './display'
import type { ObjectWithToJsonMethod } from './utils'
import { detachToJsonMethod, ONE_KIBI_BYTE } from './utils'

Expand All @@ -8,7 +9,7 @@ type ExtendedContextValue = PrimitivesAndFunctions | object | ExtendedContext |
type ExtendedContext = { [key: string]: ExtendedContextValue }
type ExtendedContextArray = ExtendedContextValue[]

type ContainerElementsToProcess = {
type ContainerElementToProcess = {
source: ExtendedContextArray | ExtendedContext
target: ContextArray | Context
path: string
Expand Down Expand Up @@ -48,7 +49,7 @@ export function sanitize(source: unknown, maxCharacterCount = SANITIZE_DEFAULT_M
const restoreArrayPrototypeToJson = detachToJsonMethod(Array.prototype)

// Initial call to sanitizeProcessor - will populate containerQueue if source is an Array or a plain Object
const containerQueue: ContainerElementsToProcess[] = []
const containerQueue: ContainerElementToProcess[] = []
const visitedObjectsWithPath = new WeakMap<object, string>()
const sanitizedData = sanitizeProcessor(
source as ExtendedContextValue,
yannickadam marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -57,12 +58,12 @@ export function sanitize(source: unknown, maxCharacterCount = SANITIZE_DEFAULT_M
containerQueue,
visitedObjectsWithPath
)
let accumulatedSize = JSON.stringify(sanitizedData)?.length || 0
if (accumulatedSize > maxCharacterCount) {
let accumulatedCharacterCount = JSON.stringify(sanitizedData)?.length || 0
if (isOverCharacterLimit(accumulatedCharacterCount, maxCharacterCount, true, source)) {
yannickadam marked this conversation as resolved.
Show resolved Hide resolved
return undefined
}

while (containerQueue.length > 0 && accumulatedSize < maxCharacterCount) {
while (containerQueue.length > 0 && accumulatedCharacterCount < maxCharacterCount) {
const containerToProcess = containerQueue.shift()!
let separatorLength = 0 // 0 for the first element, 1 for subsequent elements

Expand All @@ -77,8 +78,8 @@ export function sanitize(source: unknown, maxCharacterCount = SANITIZE_DEFAULT_M
containerQueue,
visitedObjectsWithPath
)
accumulatedSize += JSON.stringify(targetData).length + separatorLength
if (accumulatedSize > maxCharacterCount) {
accumulatedCharacterCount += JSON.stringify(targetData).length + separatorLength
if (isOverCharacterLimit(accumulatedCharacterCount, maxCharacterCount, false, source)) {
break
}
separatorLength = 1
Expand All @@ -94,8 +95,9 @@ export function sanitize(source: unknown, maxCharacterCount = SANITIZE_DEFAULT_M
containerQueue,
visitedObjectsWithPath
)
accumulatedSize += JSON.stringify(targetData).length + separatorLength + key.length + KEY_DECORATION_LENGTH
if (accumulatedSize > maxCharacterCount) {
accumulatedCharacterCount +=
JSON.stringify(targetData).length + separatorLength + key.length + KEY_DECORATION_LENGTH
if (isOverCharacterLimit(accumulatedCharacterCount, maxCharacterCount, false, source)) {
break
}
separatorLength = 1
Expand All @@ -121,7 +123,7 @@ function sanitizeProcessor(
source: ExtendedContextValue,
parentPath: string,
key: string | number | undefined,
queue: ContainerElementsToProcess[],
queue: ContainerElementToProcess[],
visitedObjectsWithPath: WeakMap<object, string>
) {
// Start by handling toJSON, as we want to sanitize its output
Expand Down Expand Up @@ -225,3 +227,25 @@ function tryToApplyToJSON(value: ExtendedContextValue) {

return value
}

/**
* Helper function that displays a warning when the accumulated character count is over the limit
*/
function isOverCharacterLimit(
currentCharacterCount: number,
maxCharacterCount: number,
isDiscarded: boolean,
source: unknown
) {
yannickadam marked this conversation as resolved.
Show resolved Hide resolved
if (currentCharacterCount > maxCharacterCount) {
display.warn(
`The data provided has been ${
isDiscarded ? 'discarded' : 'truncated'
} as it is over the limit of ${maxCharacterCount} characters:`,
source
)
return true
}

return false
}
2 changes: 1 addition & 1 deletion packages/logs/src/boot/logsPublicApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export function makeLogsPublicApi(startLogsImpl: StartLogs) {
createLogger: monitor((name: string, conf: LoggerConfiguration = {}) => {
customLoggers[name] = new Logger(
(...params) => handleLogStrategy(...params),
name,
sanitize(name),
conf.handler,
conf.level,
sanitize(conf.context) as object
yannickadam marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
8 changes: 4 additions & 4 deletions packages/logs/src/domain/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,15 @@ export class Logger {
}

debug(message: string, messageContext?: object) {
this.log(message, messageContext as object, StatusType.debug)
this.log(message, messageContext, StatusType.debug)
}

info(message: string, messageContext?: object) {
this.log(message, messageContext as object, StatusType.info)
this.log(message, messageContext, StatusType.info)
}

warn(message: string, messageContext?: object) {
this.log(message, messageContext as object, StatusType.warn)
this.log(message, messageContext, StatusType.warn)
}

error(message: string, messageContext?: object) {
Expand All @@ -61,7 +61,7 @@ export class Logger {
origin: ErrorSource.LOGGER,
},
}
this.log(message, combine(errorOrigin, messageContext as object), StatusType.error)
this.log(message, combine(errorOrigin, messageContext), StatusType.error)
}

setContext(context: object) {
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 @@ -119,7 +119,7 @@ export function makeRumPublicApi(

if (isExperimentalFeatureEnabled('feature_flags')) {
;(rumPublicApi as any).addFeatureFlagEvaluation = monitor((key: string, value: any) => {
addFeatureFlagEvaluationStrategy(key, value)
addFeatureFlagEvaluationStrategy(sanitize(key)!, sanitize(value))
})
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
isExperimentalFeatureEnabled,
SESSION_TIME_OUT_DELAY,
ContextHistory,
sanitize,
} from '@datadog/browser-core'
import type { LifeCycle } from '../lifeCycle'
import { LifeCycleEventType } from '../lifeCycle'
Expand Down Expand Up @@ -69,7 +68,7 @@ export function startFeatureFlagContexts(
addFeatureFlagEvaluation: (key: string, value: ContextValue) => {
const currentContext = featureFlagContexts.find()
if (currentContext) {
currentContext[key] = sanitize(value)
currentContext[key] = value
bytesCountCache = undefined
}
},
Expand Down