♻️ [RUMF-1436] instrument method improvements

packages/core/src/browser/fetchObservable.ts

@@ -1,5 +1,6 @@
+import type { InstrumentedMethodCall } from '../tools/instrumentMethod'
 import { instrumentMethod } from '../tools/instrumentMethod'
-import { callMonitored, monitor } from '../tools/monitor'
+import { monitor } from '../tools/monitor'
 import { Observable } from '../tools/observable'
 import type { ClocksState } from '../tools/utils/timeUtils'
 import { clocksNow } from '../tools/utils/timeUtils'
@@ -43,32 +44,17 @@ function createFetchObservable() {
       return
     }
 
-    const { stop } = instrumentMethod(
-      window,
-      'fetch',
-      (originalFetch) =>
-        function (input, init) {
-          let responsePromise: Promise<Response>
-
-          const context = callMonitored(beforeSend, null, [observable, input, init])
-          if (context) {
-            // casting should be `RequestInfo` but node types are ahead of DOM types, making `typecheck test/e2e` fail.
-            // it should be resolved with https://github.com/microsoft/TypeScript-DOM-lib-generator/issues/1483
-            responsePromise = originalFetch.call(this, context.input as any, context.init)
-            callMonitored(afterSend, null, [observable, responsePromise, context])
-          } else {
-            responsePromise = originalFetch.call(this, input, init)
-          }
-
-          return responsePromise
-        }
-    )
+    const { stop } = instrumentMethod(window, 'fetch', (call) => beforeSend(call, observable))
 
     return stop
   })
 }
 
-function beforeSend(observable: Observable<FetchContext>, input: unknown, init?: RequestInit) {
+function beforeSend(
+  { parameters, onPostCall }: InstrumentedMethodCall<Window, 'fetch'>,
+  observable: Observable<FetchContext>
+) {
+  const [input, init] = parameters
   const methodFromParams = (init && init.method) || (input instanceof Request && input.method)
   const method = methodFromParams ? methodFromParams.toUpperCase() : 'GET'
   const url = input instanceof Request ? input.url : normalizeUrl(String(input))
@@ -85,7 +71,11 @@ function beforeSend(observable: Observable<FetchContext>, input: unknown, init?:
 
   observable.notify(context)
 
-  return context
+  // Those properties can be changed by observable subscribers
+  parameters[0] = context.input as RequestInfo | URL
+  parameters[1] = context.init
+
+  onPostCall((responsePromise) => afterSend(observable, responsePromise, context))
 }
 
 function afterSend(

packages/core/src/browser/xhrObservable.ts

@@ -1,4 +1,5 @@
-import { instrumentMethodAndCallOriginal } from '../tools/instrumentMethod'
+import type { InstrumentedMethodCall } from '../tools/instrumentMethod'
+import { instrumentMethod } from '../tools/instrumentMethod'
 import { Observable } from '../tools/observable'
 import type { Duration, ClocksState } from '../tools/utils/timeUtils'
 import { elapsed, clocksNow, timeStampNow } from '../tools/utils/timeUtils'
@@ -40,19 +41,13 @@ export function initXhrObservable(configuration: Configuration) {
 
 function createXhrObservable(configuration: Configuration) {
   return new Observable<XhrContext>((observable) => {
-    const { stop: stopInstrumentingStart } = instrumentMethodAndCallOriginal(XMLHttpRequest.prototype, 'open', {
-      before: openXhr,
-    })
+    const { stop: stopInstrumentingStart } = instrumentMethod(XMLHttpRequest.prototype, 'open', openXhr)
 
-    const { stop: stopInstrumentingSend } = instrumentMethodAndCallOriginal(XMLHttpRequest.prototype, 'send', {
-      before() {
-        sendXhr.call(this, configuration, observable)
-      },
+    const { stop: stopInstrumentingSend } = instrumentMethod(XMLHttpRequest.prototype, 'send', (call) => {
+      sendXhr(call, configuration, observable)
     })
 
-    const { stop: stopInstrumentingAbort } = instrumentMethodAndCallOriginal(XMLHttpRequest.prototype, 'abort', {
-      before: abortXhr,
-    })
+    const { stop: stopInstrumentingAbort } = instrumentMethod(XMLHttpRequest.prototype, 'abort', abortXhr)
 
     return () => {
       stopInstrumentingStart()
@@ -62,16 +57,20 @@ function createXhrObservable(configuration: Configuration) {
   })
 }
 
-function openXhr(this: XMLHttpRequest, method: string, url: string | URL | undefined | null) {
-  xhrContexts.set(this, {
+function openXhr({ target: xhr, parameters: [method, url] }: InstrumentedMethodCall<XMLHttpRequest, 'open'>) {
+  xhrContexts.set(xhr, {
     state: 'open',
     method: method.toUpperCase(),
     url: normalizeUrl(String(url)),
   })
 }
 
-function sendXhr(this: XMLHttpRequest, configuration: Configuration, observable: Observable<XhrContext>) {
-  const context = xhrContexts.get(this)
+function sendXhr(
+  { target: xhr }: InstrumentedMethodCall<XMLHttpRequest, 'send'>,
+  configuration: Configuration,
+  observable: Observable<XhrContext>
+) {
+  const context = xhrContexts.get(xhr)
   if (!context) {
     return
   }
@@ -80,20 +79,18 @@ function sendXhr(this: XMLHttpRequest, configuration: Configuration, observable:
   startContext.state = 'start'
   startContext.startClocks = clocksNow()
   startContext.isAborted = false
-  startContext.xhr = this
+  startContext.xhr = xhr
 
   let hasBeenReported = false
 
-  const { stop: stopInstrumentingOnReadyStateChange } = instrumentMethodAndCallOriginal(this, 'onreadystatechange', {
-    before() {
-      if (this.readyState === XMLHttpRequest.DONE) {
-        // Try to report the XHR as soon as possible, because the XHR may be mutated by the
-        // application during a future event. For example, Angular is calling .abort() on
-        // completed requests during an onreadystatechange event, so the status becomes '0'
-        // before the request is collected.
-        onEnd()
-      }
-    },
+  const { stop: stopInstrumentingOnReadyStateChange } = instrumentMethod(xhr, 'onreadystatechange', () => {
+    if (xhr.readyState === XMLHttpRequest.DONE) {
+      // Try to report the XHR as soon as possible, because the XHR may be mutated by the
+      // application during a future event. For example, Angular is calling .abort() on
+      // completed requests during an onreadystatechange event, so the status becomes '0'
+      // before the request is collected.
+      onEnd()
+    }
   })
 
   const onEnd = () => {
@@ -107,17 +104,17 @@ function sendXhr(this: XMLHttpRequest, configuration: Configuration, observable:
     const completeContext = context as XhrCompleteContext
     completeContext.state = 'complete'
     completeContext.duration = elapsed(startContext.startClocks.timeStamp, timeStampNow())
-    completeContext.status = this.status
+    completeContext.status = xhr.status
     observable.notify(shallowClone(completeContext))
   }
 
-  const { stop: unsubscribeLoadEndListener } = addEventListener(configuration, this, 'loadend', onEnd)
+  const { stop: unsubscribeLoadEndListener } = addEventListener(configuration, xhr, 'loadend', onEnd)
 
   observable.notify(startContext)
 }
 
-function abortXhr(this: XMLHttpRequest) {
-  const context = xhrContexts.get(this) as XhrStartContext | undefined
+function abortXhr({ target: xhr }: InstrumentedMethodCall<XMLHttpRequest, 'abort'>) {
+  const context = xhrContexts.get(xhr) as XhrStartContext | undefined
   if (context) {
     context.isAborted = true
   }

packages/core/src/domain/error/trackRuntimeError.ts

@@ -1,4 +1,4 @@
-import { instrumentMethodAndCallOriginal } from '../../tools/instrumentMethod'
+import { instrumentMethod } from '../../tools/instrumentMethod'
 import type { Observable } from '../../tools/observable'
 import { clocksNow } from '../../tools/utils/timeUtils'
 import type { StackTrace } from './computeStackTrace'
@@ -33,25 +33,21 @@ export function trackRuntimeError(errorObservable: Observable<RawError>) {
 }
 
 export function instrumentOnError(callback: UnhandledErrorCallback) {
-  return instrumentMethodAndCallOriginal(window, 'onerror', {
-    before(this: any, messageObj: unknown, url?: string, line?: number, column?: number, errorObj?: unknown) {
-      let stackTrace
-      if (errorObj instanceof Error) {
-        stackTrace = computeStackTrace(errorObj)
-      } else {
-        stackTrace = computeStackTraceFromOnErrorMessage(messageObj, url, line, column)
-      }
-      callback(stackTrace, errorObj ?? messageObj)
-    },
+  return instrumentMethod(window, 'onerror', ({ parameters: [messageObj, url, line, column, errorObj] }) => {
+    let stackTrace
+    if (errorObj instanceof Error) {
+      stackTrace = computeStackTrace(errorObj)
+    } else {
+      stackTrace = computeStackTraceFromOnErrorMessage(messageObj, url, line, column)
+    }
+    callback(stackTrace, errorObj ?? messageObj)
   })
 }
 
 export function instrumentUnhandledRejection(callback: UnhandledErrorCallback) {
-  return instrumentMethodAndCallOriginal(window, 'onunhandledrejection', {
-    before(e: PromiseRejectionEvent) {
-      const reason = e.reason || 'Empty reason'
-      const stack = computeStackTrace(reason)
-      callback(stack, reason)
-    },
+  return instrumentMethod(window, 'onunhandledrejection', ({ parameters: [e] }) => {
+    const reason = e.reason || 'Empty reason'
+    const stack = computeStackTrace(reason)
+    callback(stack, reason)
   })
 }

packages/core/src/index.ts

@@ -73,7 +73,7 @@ export * from './tools/utils/browserDetection'
 export { sendToExtension } from './tools/sendToExtension'
 export { runOnReadyState } from './browser/runOnReadyState'
 export { getZoneJsOriginalValue } from './tools/getZoneJsOriginalValue'
-export { instrumentMethod, instrumentMethodAndCallOriginal, instrumentSetter } from './tools/instrumentMethod'
+export { instrumentMethod, instrumentSetter, InstrumentedMethodCall } from './tools/instrumentMethod'
 export {
   computeRawError,
   createHandlingStack,

packages/core/src/tools/instrumentMethod.spec.ts

@@ -1,69 +1,83 @@
 import { mockClock, stubZoneJs } from '../../test'
 import type { Clock } from '../../test'
+import type { InstrumentedMethodCall } from './instrumentMethod'
 import { instrumentMethod, instrumentSetter } from './instrumentMethod'
 import { noop } from './utils/functionUtils'
 
 describe('instrumentMethod', () => {
+  const THIRD_PARTY_RESULT = 42
+
   it('replaces the original method', () => {
     const original = () => 1
     const object = { method: original }
 
-    instrumentMethod(object, 'method', () => () => 2)
+    instrumentMethod(object, 'method', noop)
 
     expect(object.method).not.toBe(original)
-    expect(object.method()).toBe(2)
   })
 
-  it('sets a method originally undefined', () => {
-    const object: { method?: () => number } = {}
+  it('calls the instrumentation before the original method', () => {
+    const originalSpy = jasmine.createSpy()
+    const instrumentationSpy = jasmine.createSpy()
+    const object = { method: originalSpy }
 
-    instrumentMethod(object, 'method', () => () => 2)
+    instrumentMethod(object, 'method', instrumentationSpy)
 
-    expect(object.method!()).toBe(2)
+    object.method()
+
+    expect(instrumentationSpy).toHaveBeenCalledBefore(originalSpy)
   })
 
-  it('provides the original method to the instrumentation factory', () => {
-    const original = () => 1
-    const object = { method: original }
-    const instrumentationFactorySpy = jasmine.createSpy().and.callFake((original: () => number) => () => original() + 2)
+  it('sets a method originally undefined', () => {
+    const object: { method?: () => number } = {}
+
+    const instrumentationSpy = jasmine.createSpy()
+    instrumentMethod(object, 'method', instrumentationSpy)
 
-    instrumentMethod(object, 'method', instrumentationFactorySpy)
+    expect(object.method).toBeDefined()
+    object.method!()
 
-    expect(instrumentationFactorySpy).toHaveBeenCalledOnceWith(original)
-    expect(object.method()).toBe(3)
+    expect(instrumentationSpy).toHaveBeenCalled()
   })
 
-  it('calls the instrumentation with method arguments', () => {
+  it('calls the instrumentation with method target and parameters', () => {
     const object = { method: (a: number, b: number) => a + b }
-    const instrumentationSpy = jasmine.createSpy()
-    instrumentMethod(object, 'method', () => instrumentationSpy)
+    const instrumentationSpy = jasmine.createSpy<(call: InstrumentedMethodCall<typeof object, 'method'>) => void>()
+    instrumentMethod(object, 'method', instrumentationSpy)
 
     object.method(2, 3)
 
-    expect(instrumentationSpy).toHaveBeenCalledOnceWith(2, 3)
+    expect(instrumentationSpy).toHaveBeenCalledOnceWith({
+      target: object,
+      parameters: jasmine.any(Object),
+      onPostCall: jasmine.any(Function),
+    })
+    expect(instrumentationSpy.calls.mostRecent().args[0].parameters[0]).toBe(2)
+    expect(instrumentationSpy.calls.mostRecent().args[0].parameters[1]).toBe(3)
+  })
+
+  it('calls the "onPostCall" callback with the original method result', () => {
+    const object = { method: () => 1 }
+    const onPostCallSpy = jasmine.createSpy()
+    instrumentMethod(object, 'method', ({ onPostCall }) => onPostCall(onPostCallSpy))
+
+    object.method()
+
+    expect(onPostCallSpy).toHaveBeenCalledOnceWith(1)
   })
 
   it('allows other instrumentations from third parties', () => {
     const object = { method: () => 1 }
-    const instrumentationSpy = jasmine.createSpy().and.returnValue(2)
-    instrumentMethod(object, 'method', () => instrumentationSpy)
+    const instrumentationSpy = jasmine.createSpy()
+    instrumentMethod(object, 'method', instrumentationSpy)
 
     thirdPartyInstrumentation(object)
 
-    expect(object.method()).toBe(4)
+    expect(object.method()).toBe(THIRD_PARTY_RESULT)
     expect(instrumentationSpy).toHaveBeenCalled()
   })
 
   describe('stop()', () => {
-    it('restores the original behavior', () => {
-      const object = { method: () => 1 }
-      const { stop } = instrumentMethod(object, 'method', () => () => 2)
-
-      stop()
-
-      expect(object.method()).toBe(1)
-    })
-
     it('does not call the instrumentation anymore', () => {
       const object = { method: () => 1 }
       const instrumentationSpy = jasmine.createSpy()
@@ -78,7 +92,7 @@ describe('instrumentMethod', () => {
     describe('when the method has been instrumented by a third party', () => {
       it('should not break the third party instrumentation', () => {
         const object = { method: () => 1 }
-        const { stop } = instrumentMethod(object, 'method', () => () => 2)
+        const { stop } = instrumentMethod(object, 'method', noop)
 
         thirdPartyInstrumentation(object)
         const instrumentedMethod = object.method
@@ -91,7 +105,7 @@ describe('instrumentMethod', () => {
       it('does not call the instrumentation', () => {
         const object = { method: () => 1 }
         const instrumentationSpy = jasmine.createSpy()
-        const { stop } = instrumentMethod(object, 'method', () => instrumentationSpy)
+        const { stop } = instrumentMethod(object, 'method', instrumentationSpy)
 
         thirdPartyInstrumentation(object)
 
@@ -103,7 +117,7 @@ describe('instrumentMethod', () => {
       it('should not throw errors if original method was undefined', () => {
         const object: { method?: () => number } = {}
         const instrumentationStub = () => 2
-        const { stop } = instrumentMethod(object, 'method', () => instrumentationStub)
+        const { stop } = instrumentMethod(object, 'method', instrumentationStub)
 
         thirdPartyInstrumentation(object)
 
@@ -117,7 +131,10 @@ describe('instrumentMethod', () => {
   function thirdPartyInstrumentation(object: { method?: () => number }) {
     const originalMethod = object.method
     if (typeof originalMethod === 'function') {
-      object.method = () => originalMethod() + 2
+      object.method = () => {
+        originalMethod()
+        return THIRD_PARTY_RESULT
+      }
     }
   }
 })