🎨 Instrument fetch and XHR before trying to init consent

[N-Boutaib]

Motivation

When RUM's initialisation is delayed because of consent not being granted initially, some libraries (such as Apollo Client) do store some methods aside to be re-used (mainly window.fetch), which leads to the un-instrumented method being used for http requests, even after consent being granted.

Changes

Instrument fetch method on sdk initialisation. The instrumentation process is idempotent.

Testing

• Local
• Staging
• Unit
• End to end

---

I have gone over the contributing documentation.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.09%. Comparing base (fab7ead) to head (14807bd).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2834      +/-   ##
==========================================
- Coverage   93.10%   93.09%   -0.02%     
==========================================
  Files         248      248              
  Lines        7242     7246       +4     
  Branches     1624     1624              
==========================================
+ Hits         6743     6746       +3     
- Misses        499      500       +1     

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

[cit-pr-commenter]

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	160.40 KiB	160.42 KiB	18 B	+0.01%	✅
Logs	58.02 KiB	58.04 KiB	18 B	+0.03%	✅
Rum Slim	108.92 KiB	108.94 KiB	18 B	+0.02%	✅
Worker	25.21 KiB	25.21 KiB	0 B	0.00%	✅

🚀 CPU Performance

Action Name	Base Average Cpu Time (ms)	Local Average Cpu Time (ms)	𝚫
addglobalcontext	0.001	0.002	0.000
addaction	0.031	0.037	0.006
adderror	0.032	0.035	0.003
addtiming	0.001	0.001	0.000
startview	0.898	0.998	0.100
startstopsessionreplayrecording	0.860	0.952	0.092
logmessage	0.019	0.021	0.002

🧠 Memory Performance

Action Name	Base Consumption Memory (bytes)	Local Consumption Memory (bytes)	𝚫 (bytes)
addglobalcontext	19.53 KiB	17.88 KiB	-1698 B
addaction	72.93 KiB	68.45 KiB	-4583 B
adderror	86.17 KiB	85.68 KiB	-500 B
addtiming	19.72 KiB	16.54 KiB	-3251 B
startview	314.43 KiB	320.54 KiB	6.11 KiB
startstopsessionreplayrecording	15.25 KiB	14.13 KiB	-1155 B
logmessage	71.76 KiB	69.40 KiB	-2424 B

🔗 RealWorld

[thomas-lebeau]

🔨 warning: ‏ I think we have the same issue with logs, don't we?

❓ question: ‏ What about other instrumented methods? It might be less common for them to be used that way, but we could have the same issue if that happens, no?

[BenoitZugmeyer]

🔨 warning: ‏ I think we have the same issue with logs, don't we?

❓ question: ‏ What about other instrumented methods? It might be less common for them to be used that way, but we could have the same issue if that happens, no?

For both subjects: I would wait for requests to do so before acting on it, as we don't want to over-instrument if we don't need to (in other words, it is better to not instrument if we never have user consent).

[BenoitZugmeyer]

💬 suggestion: ‏don't disable ESLint rules for the whole file. You can wrap your code between comments:

/* eslint-disable ... */
xxx
/* eslint-enable ... */

[BenoitZugmeyer]

💬 suggestion: ‏move this at the start of your test case.
Some other tests executing earlier could instrument those methods and not properly clean them up.

[N-Boutaib]

/to-staging

[dd-devflow]

🚂 Branch Integration: starting soon, median merge time is 10m

Commit d3f3bd6f78 will soon be integrated into staging-26.

Use /to-staging -c to cancel this operation!

[dd-devflow]

⚠️ Branch Integration: This commit was already integrated

Commit d3f3bd6f78 had already been merged into staging-26

If you need support, contact us on Slack #devflow!

[amortemousque]

💬 suggestion:
Thesubscribe(noop) feels a bit hacky.
You could change the createFetchObservable as follow to avoid having to call the subscribe.

function createFetchObservable() {
  let instrumentation: (call: any) => void = noop
  const { stop } = instrumentMethod(window, 'fetch', (call) => instrumentation(call), { computeHandlingStack: true })

  return new Observable<FetchContext>((observable) => {
    if (!window.fetch) {
      return
    }
    instrumentation = (call) => beforeSend(call, observable)

    return stop
  })
}

[BenoitZugmeyer]

We talked a bit about it during standup. I'll try to sum it up here.

On one side, we all agree that .subscribe(noop) is a bit suprising (but not necessarily hacky). On the other side, changing this would make things more complex in fetchObservable.ts.

What you are suggesting is incorrect because if stop is called because there is no more subscriber, then the observable is subscribed again, we wouldn't re-instrument fetch. In practice this probably does not happen, but for a newcomer reading this code without context this might be seen as a bug.

So we would need another way to clean the instrumentation? Or maybe don't clean the instrumentation? It doesn't feel great either.

[amortemousque]

Well I think .subscribe(noop) does not convey the intention of what it does and raise more questions about the implementation for new newcomer reading this code.
I put the stop in the observable only to avoid modifying too much the tests. We could have also extended the resetFetchObservable to reset the instrumentation. Since the removal of the instrumentation and the reset of the observable are only done for testing purposes, this approach would have been reasonable. I feel like we're choosing a confusing implementation just to avoid refactoring the tests.