From 53dbbbc6990db0814f3e4df38051d9d1348c64fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Fri, 11 Mar 2022 11:18:15 +0100 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=90=9B=20[RUMF-1203]=20use=20`instrum?= =?UTF-8?q?entMethod`=20for=20CSSStyleSheet=20methods?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/core/src/tools/instrumentMethod.ts | 4 +-- packages/rum/src/domain/record/observer.ts | 28 ++++++++------------- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/packages/core/src/tools/instrumentMethod.ts b/packages/core/src/tools/instrumentMethod.ts index 946283893a..e41df25231 100644 --- a/packages/core/src/tools/instrumentMethod.ts +++ b/packages/core/src/tools/instrumentMethod.ts @@ -35,8 +35,8 @@ export function instrumentMethodAndCallOriginal) => ReturnType - after?: (this: OBJECT, ...args: Parameters) => ReturnType + before?: (this: OBJECT, ...args: Parameters) => void + after?: (this: OBJECT, ...args: Parameters) => void } ) { return instrumentMethod( diff --git a/packages/rum/src/domain/record/observer.ts b/packages/rum/src/domain/record/observer.ts index 8980128f04..1c9c0f7861 100644 --- a/packages/rum/src/domain/record/observer.ts +++ b/packages/rum/src/domain/record/observer.ts @@ -1,8 +1,8 @@ import type { DefaultPrivacyLevel } from '@datadog/browser-core' import { + instrumentMethodAndCallOriginal, assign, monitor, - callMonitored, throttle, DOM_EVENT, addEventListeners, @@ -308,37 +308,31 @@ export function initInputObserver(cb: InputCallback, defaultPrivacyLevel: Defaul } function initStyleSheetObserver(cb: StyleSheetRuleCallback): ListenerHandler { - // eslint-disable-next-line @typescript-eslint/unbound-method - const insertRule = CSSStyleSheet.prototype.insertRule - CSSStyleSheet.prototype.insertRule = function (this: CSSStyleSheet, rule: string, index?: number) { - callMonitored(() => { + const { stop: restoreInsertRule } = instrumentMethodAndCallOriginal(CSSStyleSheet.prototype, 'insertRule', { + before(rule, index) { if (hasSerializedNode(this.ownerNode!)) { cb({ id: getSerializedNodeId(this.ownerNode), adds: [{ rule, index }], }) } - }) - return insertRule.call(this, rule, index) - } + }, + }) - // eslint-disable-next-line @typescript-eslint/unbound-method - const deleteRule = CSSStyleSheet.prototype.deleteRule - CSSStyleSheet.prototype.deleteRule = function (this: CSSStyleSheet, index: number) { - callMonitored(() => { + const { stop: restoreDeleteRule } = instrumentMethodAndCallOriginal(CSSStyleSheet.prototype, 'deleteRule', { + before(index) { if (hasSerializedNode(this.ownerNode!)) { cb({ id: getSerializedNodeId(this.ownerNode), removes: [{ index }], }) } - }) - return deleteRule.call(this, index) - } + }, + }) return () => { - CSSStyleSheet.prototype.insertRule = insertRule - CSSStyleSheet.prototype.deleteRule = deleteRule + restoreInsertRule() + restoreDeleteRule() } } From 9eeabe8c7f70f399a1fab6ea83c6f67fc3b86fda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Fri, 11 Mar 2022 12:49:32 +0100 Subject: [PATCH 2/4] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20[RUMF-1203]=20simplify?= =?UTF-8?q?=20input=20observer=20a=20bit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/rum/src/domain/record/observer.ts | 45 ++++++++++++---------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/packages/rum/src/domain/record/observer.ts b/packages/rum/src/domain/record/observer.ts index 1c9c0f7861..916d7302b3 100644 --- a/packages/rum/src/domain/record/observer.ts +++ b/packages/rum/src/domain/record/observer.ts @@ -7,7 +7,6 @@ import { DOM_EVENT, addEventListeners, addEventListener, - includes, noop, } from '@datadog/browser-core' import { NodePrivacyLevel } from '../../constants' @@ -201,18 +200,12 @@ function initViewportResizeObserver(cb: ViewportResizeCallback): ListenerHandler return addEventListener(window, DOM_EVENT.RESIZE, updateDimension, { capture: true, passive: true }).stop } -export const INPUT_TAGS = ['INPUT', 'TEXTAREA', 'SELECT'] -const lastInputStateMap: WeakMap = new WeakMap() export function initInputObserver(cb: InputCallback, defaultPrivacyLevel: DefaultPrivacyLevel): ListenerHandler { - function eventHandler(event: { target: EventTarget | null }) { - const target = event.target as HTMLInputElement | HTMLTextAreaElement + const lastInputStateMap: WeakMap = new WeakMap() + + function onElementChange(target: HTMLInputElement | HTMLTextAreaElement | HTMLSelectElement) { const nodePrivacyLevel = getNodePrivacyLevel(target, defaultPrivacyLevel) - if ( - !target || - !target.tagName || - !includes(INPUT_TAGS, target.tagName) || - nodePrivacyLevel === NodePrivacyLevel.HIDDEN - ) { + if (nodePrivacyLevel === NodePrivacyLevel.HIDDEN) { return } @@ -272,29 +265,41 @@ export function initInputObserver(cb: InputCallback, defaultPrivacyLevel: Defaul } } - const { stop: stopEventListeners } = addEventListeners(document, [DOM_EVENT.INPUT, DOM_EVENT.CHANGE], eventHandler, { - capture: true, - passive: true, - }) + const { stop: stopEventListeners } = addEventListeners( + document, + [DOM_EVENT.INPUT, DOM_EVENT.CHANGE], + (event) => { + if ( + event.target instanceof HTMLInputElement || + event.target instanceof HTMLTextAreaElement || + event.target instanceof HTMLSelectElement + ) { + onElementChange(event.target) + } + }, + { + capture: true, + passive: true, + } + ) const propertyDescriptor = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value') - const hookProperties: Array<[HTMLElement, string]> = [ + const hookProperties = [ [HTMLInputElement.prototype, 'value'], [HTMLInputElement.prototype, 'checked'], [HTMLSelectElement.prototype, 'value'], [HTMLTextAreaElement.prototype, 'value'], // Some UI library use selectedIndex to set select value [HTMLSelectElement.prototype, 'selectedIndex'], - ] + ] as const const hookResetters: HookResetter[] = [] if (propertyDescriptor && propertyDescriptor.set) { hookResetters.push( ...hookProperties.map((p) => - hookSetter(p[0], p[1], { + hookSetter(p[0], p[1], { set: monitor(function () { - // mock to a normal event - eventHandler({ target: this }) + onElementChange(this) }), }) ) From 5ac4acc6bc235cfc1bd9ea35bff28f9b68f84127 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Tue, 22 Mar 2022 16:21:50 +0100 Subject: [PATCH 3/4] =?UTF-8?q?=E2=9C=A8=20[RUMF-1203]=20implement=20a=20i?= =?UTF-8?q?nstrumentSetter=20similar=20to=20instrumentMethod?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/core/src/index.ts | 2 +- .../core/src/tools/instrumentMethod.spec.ts | 157 +++++++++++++++++- packages/core/src/tools/instrumentMethod.ts | 43 ++++- 3 files changed, 199 insertions(+), 3 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9584797936..926e7f94e5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -44,7 +44,7 @@ export * from './tools/timeUtils' export * from './tools/utils' export * from './tools/createEventRateLimiter' export * from './tools/browserDetection' -export { instrumentMethod, instrumentMethodAndCallOriginal } from './tools/instrumentMethod' +export { instrumentMethod, instrumentMethodAndCallOriginal, instrumentSetter } from './tools/instrumentMethod' export { ErrorSource, ErrorHandling, diff --git a/packages/core/src/tools/instrumentMethod.spec.ts b/packages/core/src/tools/instrumentMethod.spec.ts index 4601dfc389..b64111af85 100644 --- a/packages/core/src/tools/instrumentMethod.spec.ts +++ b/packages/core/src/tools/instrumentMethod.spec.ts @@ -1,4 +1,7 @@ -import { instrumentMethod } from './instrumentMethod' +import type { Clock } from '../../test/specHelper' +import { mockClock } from '../../test/specHelper' +import { instrumentMethod, instrumentSetter } from './instrumentMethod' +import { noop } from './utils' describe('instrumentMethod', () => { it('replaces the original method', () => { @@ -104,3 +107,155 @@ describe('instrumentMethod', () => { object.method = () => originalMethod() + 2 } }) + +describe('instrumentSetter', () => { + let clock: Clock + beforeEach(() => { + clock = mockClock() + }) + afterEach(() => { + clock.cleanup() + }) + + it('replaces the original setter', () => { + const originalSetter = () => { + // do nothing particular, only used to test if this setter gets replaced + } + const object = {} as { foo: number } + Object.defineProperty(object, 'foo', { set: originalSetter, configurable: true }) + + instrumentSetter(object, 'foo', noop) + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).not.toBe(originalSetter) + }) + + it('skips instrumentation if there is no original setter', () => { + const object = { foo: 1 } + + instrumentSetter(object, 'foo', noop) + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).toBeUndefined() + }) + + it('skips instrumentation if the descriptor is not configurable', () => { + const originalSetter = () => { + // do nothing particular, only used to test if this setter gets replaced + } + const object = {} as { foo: number } + Object.defineProperty(object, 'foo', { set: originalSetter, configurable: false }) + + instrumentSetter(object, 'foo', noop) + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).toBe(originalSetter) + }) + + it('calls the original setter', () => { + const originalSetterSpy = jasmine.createSpy() + const object = {} as { foo: number } + Object.defineProperty(object, 'foo', { set: originalSetterSpy, configurable: true }) + + instrumentSetter(object, 'foo', noop) + + object.foo = 1 + expect(originalSetterSpy).toHaveBeenCalledOnceWith(1) + }) + + it('calls the instrumentation asynchronously', () => { + const instrumentationSetterSpy = jasmine.createSpy() + const object = {} as { foo: number } + Object.defineProperty(object, 'foo', { set: noop, configurable: true }) + + instrumentSetter(object, 'foo', instrumentationSetterSpy) + + object.foo = 1 + expect(instrumentationSetterSpy).not.toHaveBeenCalled() + clock.tick(0) + expect(instrumentationSetterSpy).toHaveBeenCalledOnceWith(object, 1) + }) + + it('allows other instrumentations from third parties', () => { + const object = {} as { foo: number } + Object.defineProperty(object, 'foo', { set: noop, configurable: true }) + const instrumentationSetterSpy = jasmine.createSpy() + instrumentSetter(object, 'foo', instrumentationSetterSpy) + + const thirdPartyInstrumentationSpy = thirdPartyInstrumentation(object) + + object.foo = 2 + expect(thirdPartyInstrumentationSpy).toHaveBeenCalledOnceWith(2) + clock.tick(0) + expect(instrumentationSetterSpy).toHaveBeenCalledOnceWith(object, 2) + }) + + describe('stop()', () => { + it('restores the original behavior', () => { + const object = {} as { foo: number } + const originalSetter = () => { + // do nothing particular, only used to test if this setter gets replaced + } + Object.defineProperty(object, 'foo', { set: originalSetter, configurable: true }) + const { stop } = instrumentSetter(object, 'foo', noop) + + stop() + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).toBe(originalSetter) + }) + + it('does not call the instrumentation anymore', () => { + const object = {} as { foo: number } + Object.defineProperty(object, 'foo', { set: noop, configurable: true }) + const instrumentationSetterSpy = jasmine.createSpy() + const { stop } = instrumentSetter(object, 'foo', instrumentationSetterSpy) + + stop() + + object.foo = 2 + + expect(instrumentationSetterSpy).not.toHaveBeenCalled() + }) + + describe('when the method has been instrumented by a third party', () => { + it('should not break the third party instrumentation', () => { + const object = {} as { foo: number } + Object.defineProperty(object, 'foo', { set: noop, configurable: true }) + const { stop } = instrumentSetter(object, 'foo', noop) + + const thirdPartyInstrumentationSpy = thirdPartyInstrumentation(object) + + stop() + + // eslint-disable-next-line @typescript-eslint/unbound-method + expect(Object.getOwnPropertyDescriptor(object, 'foo')!.set).toBe(thirdPartyInstrumentationSpy) + }) + + it('does not call the instrumentation', () => { + const object = {} as { foo: number } + Object.defineProperty(object, 'foo', { set: noop, configurable: true }) + const instrumentationSetterSpy = jasmine.createSpy() + const { stop } = instrumentSetter(object, 'foo', noop) + + thirdPartyInstrumentation(object) + + stop() + + object.foo = 2 + + expect(instrumentationSetterSpy).not.toHaveBeenCalled() + }) + }) + }) + + function thirdPartyInstrumentation(object: { foo: number }) { + // eslint-disable-next-line @typescript-eslint/unbound-method + const originalSetter = Object.getOwnPropertyDescriptor(object, 'foo')!.set + const thirdPartyInstrumentationSpy = jasmine.createSpy().and.callFake(function (this: any, value) { + if (originalSetter) { + originalSetter.call(this, value) + } + }) + Object.defineProperty(object, 'foo', { set: thirdPartyInstrumentationSpy }) + return thirdPartyInstrumentationSpy + } +}) diff --git a/packages/core/src/tools/instrumentMethod.ts b/packages/core/src/tools/instrumentMethod.ts index e41df25231..dba422c2ea 100644 --- a/packages/core/src/tools/instrumentMethod.ts +++ b/packages/core/src/tools/instrumentMethod.ts @@ -1,4 +1,5 @@ -import { callMonitored } from '../domain/internalMonitoring' +import { callMonitored, monitor } from '../domain/internalMonitoring' +import { noop } from './utils' export function instrumentMethod( object: OBJECT, @@ -65,3 +66,43 @@ export function instrumentMethodAndCallOriginal( + object: OBJECT, + property: PROPERTY, + after: (thisObject: OBJECT, value: OBJECT[PROPERTY]) => void +) { + const originalDescriptor = Object.getOwnPropertyDescriptor(object, property) + if (!originalDescriptor || !originalDescriptor.set || !originalDescriptor.configurable) { + return { stop: noop } + } + + let instrumentation = (thisObject: OBJECT, value: OBJECT[PROPERTY]) => { + // put hooked setter into event loop to avoid of set latency + setTimeout( + monitor(() => { + after(thisObject, value) + }), + 0 + ) + } + + const instrumentationWrapper = function (this: OBJECT, value: OBJECT[PROPERTY]) { + originalDescriptor.set!.call(this, value) + instrumentation(this, value) + } + + Object.defineProperty(object, property, { + set: instrumentationWrapper, + }) + + return { + stop: () => { + if (Object.getOwnPropertyDescriptor(object, property)?.set === instrumentationWrapper) { + Object.defineProperty(object, property, originalDescriptor) + } else { + instrumentation = noop + } + }, + } +} From 5d9c8e69f6fef377a90a29de6a86b5ed694c4f07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20Zugmeyer?= Date: Tue, 22 Mar 2022 16:23:25 +0100 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=90=9B=20[RUMF-1203]=20replace=20`hoo?= =?UTF-8?q?kSetter`=20with=20the=20new=20`instrumentSetter`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/rum/src/domain/record/observer.ts | 35 +++++++--------------- packages/rum/src/domain/record/types.ts | 1 - packages/rum/src/domain/record/utils.ts | 24 --------------- 3 files changed, 10 insertions(+), 50 deletions(-) diff --git a/packages/rum/src/domain/record/observer.ts b/packages/rum/src/domain/record/observer.ts index 916d7302b3..9dcc18bfa0 100644 --- a/packages/rum/src/domain/record/observer.ts +++ b/packages/rum/src/domain/record/observer.ts @@ -1,5 +1,6 @@ import type { DefaultPrivacyLevel } from '@datadog/browser-core' import { + instrumentSetter, instrumentMethodAndCallOriginal, assign, monitor, @@ -14,7 +15,6 @@ import { getNodePrivacyLevel, shouldMaskNode } from './privacy' import { getElementInputValue, getSerializedNodeId, hasSerializedNode } from './serializationUtils' import type { FocusCallback, - HookResetter, InputCallback, InputState, ListenerHandler, @@ -31,7 +31,7 @@ import type { MouseInteractionParam, } from './types' import { IncrementalSource, MediaInteractions, MouseInteractions } from './types' -import { forEach, hookSetter, isTouchEvent } from './utils' +import { forEach, isTouchEvent } from './utils' import type { MutationController } from './mutationObserver' import { startMutationObserver } from './mutationObserver' @@ -283,31 +283,16 @@ export function initInputObserver(cb: InputCallback, defaultPrivacyLevel: Defaul } ) - const propertyDescriptor = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value') - const hookProperties = [ - [HTMLInputElement.prototype, 'value'], - [HTMLInputElement.prototype, 'checked'], - [HTMLSelectElement.prototype, 'value'], - [HTMLTextAreaElement.prototype, 'value'], - // Some UI library use selectedIndex to set select value - [HTMLSelectElement.prototype, 'selectedIndex'], - ] as const - - const hookResetters: HookResetter[] = [] - if (propertyDescriptor && propertyDescriptor.set) { - hookResetters.push( - ...hookProperties.map((p) => - hookSetter(p[0], p[1], { - set: monitor(function () { - onElementChange(this) - }), - }) - ) - ) - } + const instrumentationStoppers = [ + instrumentSetter(HTMLInputElement.prototype, 'value', onElementChange), + instrumentSetter(HTMLInputElement.prototype, 'checked', onElementChange), + instrumentSetter(HTMLSelectElement.prototype, 'value', onElementChange), + instrumentSetter(HTMLTextAreaElement.prototype, 'value', onElementChange), + instrumentSetter(HTMLSelectElement.prototype, 'selectedIndex', onElementChange), + ] return () => { - hookResetters.forEach((h) => h()) + instrumentationStoppers.forEach((stopper) => stopper.stop()) stopEventListeners() } } diff --git a/packages/rum/src/domain/record/types.ts b/packages/rum/src/domain/record/types.ts index 34217d2287..0fa7a0589e 100644 --- a/packages/rum/src/domain/record/types.ts +++ b/packages/rum/src/domain/record/types.ts @@ -248,7 +248,6 @@ export type FocusCallback = (data: FocusRecord['data']) => void export type VisualViewportResizeCallback = (data: VisualViewportRecord['data']) => void export type ListenerHandler = () => void -export type HookResetter = () => void export const enum NodeType { Document, diff --git a/packages/rum/src/domain/record/utils.ts b/packages/rum/src/domain/record/utils.ts index d04365d489..4151d30518 100644 --- a/packages/rum/src/domain/record/utils.ts +++ b/packages/rum/src/domain/record/utils.ts @@ -1,27 +1,3 @@ -import type { HookResetter } from './types' - -export function hookSetter( - target: T, - key: string | number | symbol, - d: { set(this: T, value: unknown): void } -): HookResetter { - const original = Object.getOwnPropertyDescriptor(target, key) - Object.defineProperty(target, key, { - set(this: T, value) { - // put hooked setter into event loop to avoid of set latency - setTimeout(() => { - d.set.call(this, value) - }, 0) - if (original && original.set) { - original.set.call(this, value) - } - }, - }) - return () => { - Object.defineProperty(target, key, original || {}) - } -} - export function isTouchEvent(event: MouseEvent | TouchEvent): event is TouchEvent { return Boolean((event as TouchEvent).changedTouches) }