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-1436] instrument method improvements #2551

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Jan 4, 2024

Motivation

In the process of fixing RUM-211 (some methods are "readonly" and their instrumentation breaks the SDK), I wanted to use the functions to instrument methods in more places. This PR is a step in that direction: it revamps and factorizes our existing intrument methods functions into a single, more easily extendable one.

Changes

This PR changes the instrumentMethodAndCallOriginal signature to call the intrumentation callback with a InstrumentedMethodCall object containing the target and parameters properties instead of passing them directly as this and function arguments.

This new InstrumentedMethodCall object also allows to:

  • register a "post call" callback that will be called with the original method result
  • mutate the parameters in-place

This allows to use instrumentMethodAndCallOriginal in fetchObservable. By doing so, the original instrumentMethod function isn't used anymore, and we can keep a single function for all our method instrumentations.

This new InstrumentedMethodCall object will also allow to transmit more information in the future, like the "handling stack" to allow using our instrumentation tooling for console.* methods.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

This change changes instrumentMethodAndCallOriginal to be more flexible
and support more use-cases. In practice, this will allow:

* replacing instrumentMethod usages with
  instrumentMethodAndCallOriginal, so we have a single function to
  instrument method.

* add more "instrumentation features" in the near future, like the
  possibility to create "handling stack traces" easily
…dCallOriginal`

Also, consolidate tests around this new implementation.
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/instrument-method-improvements branch from 2343273 to a1fa357 Compare January 4, 2024 15:52
@BenoitZugmeyer BenoitZugmeyer marked this pull request as ready for review January 4, 2024 15:54
@BenoitZugmeyer BenoitZugmeyer requested review from a team as code owners January 4, 2024 15:54
@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (49dba56) 92.83% compared to head (0ac913f) 92.82%.
Report is 1 commits behind head on main.

Files Patch % Lines
...s/rum-core/src/browser/locationChangeObservable.ts 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2551      +/-   ##
==========================================
- Coverage   92.83%   92.82%   -0.01%     
==========================================
  Files         227      227              
  Lines        6714     6706       -8     
  Branches     1480     1478       -2     
==========================================
- Hits         6233     6225       -8     
  Misses        481      481              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

BenoitZugmeyer and others added 2 commits January 5, 2024 10:21
Co-authored-by: Bastien Caudan <1331991+bcaudan@users.noreply.github.com>
@BenoitZugmeyer BenoitZugmeyer merged commit fc00aa9 into main Jan 9, 2024
17 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/instrument-method-improvements branch January 9, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants