XHR instrumentation experimental package #239
Conversation
…Faro instrumentation
…sts and more refactoring
…lliot/instrumentation-xhr
| return [ | ||
| new DocumentLoadInstrumentation(), | ||
| new FetchInstrumentation(options), | ||
| new XMLHttpRequestInstrumentation(options), | ||
| // new FetchInstrumentation(options), |
There was a problem hiding this comment.
Q: Which instrumentations (Faro vs Otel) to use is up to the user and they should not be disabled by defult.
For example you can autodetect if Faros' Fetch and Session Instrumentation are added and then disable the OTel instrumentations.
You can iterate over the instrumentation and look up for the respectiv instrumentation names, e. g. instrumentation.name === '@grafana/faro-web-sdk:instrumentation-fetch';
There was a problem hiding this comment.
We also have quite users using the Web-Tracing package.
So this will be a breaking change and we need to communicate this.
There was a problem hiding this comment.
Thanks for catching this - this was a testing change that slipped through.
The suggestion to filter out instrumentations is perfect, I will implement that. Thanks for the suggestion.
| /** | ||
| * XMLHttpRequest.prototype.open becomes instrumented and the parameter defaults are properly passed to the original function | ||
| */ | ||
| XMLHttpRequest.prototype.open = function ( |
There was a problem hiding this comment.
Suggestion: move the respective init code in a separate function each. May be a bit more readable and we wouldn't need the comments anymore.
| initialize(): void { | ||
| this.internalLogger.info('Initializing XHR instrumentation'); | ||
| this.ignoredUrls = this.options?.ignoredUrls ?? this.getTransportIgnoreUrls(); | ||
| const instrumentation = this; |
There was a problem hiding this comment.
Q: Why do you need to cache the this value of the class?
There was a problem hiding this comment.
Because this in the instrumentation class differs from this inside the scope of the XHRHttpRequest(I am pretty sure, at least)
There was a problem hiding this comment.
Ah of course, we are in class so you need the this keyword to get the function.
There was a problem hiding this comment.
What you also can do is making originalOpen open static but I am not sure if this has any big advantages to your solution with caching the this value.
Only want to add it for completeness :)
static readonly originalOpen: XMLHttpRequest['open'] = XMLHttpRequest.prototype.open;
\\ ...
return XHRInstrumentation.originalOpen.apply(this, [..])
| // @ts-expect-error - _url is attached to the xhr object | ||
| xhr._url = url; | ||
|
|
||
| return instrumentation.originalOpen?.apply(xhr, [ |
There was a problem hiding this comment.
originalOpen is in the outer scope so originalOpen.appy should work fine.
There was a problem hiding this comment.
Q: Why do we need the optional chainign operator?
In what cases will originalOpen be null | undefined?
There was a problem hiding this comment.
No need for the optional chaining operator, good catch. But my editor complains if I remove instrumentation.
| * Parse the input object into a string URL | ||
| */ | ||
| getRequestUrl(input: string | URL): string { | ||
| return input instanceof URL ? input.href : String(input); |
There was a problem hiding this comment.
Q: Why do we need to call the String() constructor if input is already a string in the 'else' case?
| XMLHttpRequest.prototype.send = function (body?: Document | XMLHttpRequestBodyInit | null | undefined): void { | ||
| const xhr = this; | ||
| // @ts-expect-error - _url is attached to the xhr object | ||
| const requestUrl = xhr._url; |
There was a problem hiding this comment.
Q: Do we need that constant? Why not use xhr._url directly? getRequestUrl(xhr._url)
| import { XHRInstrumentation } from '@grafana/faro-instrumentation-xhr'; | ||
| import { getWebInstrumentations, initializeFaro } from '@grafana/faro-react'; | ||
|
|
||
| initializeFaro({ |
There was a problem hiding this comment.
Suggestion: explain the availbale options too.
Description
Adds the XHR (XMLHttpRequest) auto-instrumentation to the experimental package
Checklist