-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[network-plugin] Feat: Capture network events #1105
base: master
Are you sure you want to change the base?
Conversation
b366ff1
to
f3d85e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With more tests, I think this pull request is good to go
This looks great! I agree with @Mark-Fenng, if we have some more tests this will be ready to merge. Also I'm happy to spend some time working on the 2.0.0-alpha.5 release soon |
Co-authored-by: Ben White <[email protected]>
|
@jlalmes need some help getting this across the finish line? This would be such a good improvement to the custom solution we have for network recording and things here look pretty solid, just needing some good tests it seems. |
Is this plugin released? |
I really need this feature。I think this is a very powerful feature @jlalmes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saw this is starting to get reviewed and wanted to throw in a thought regarding ignoreRequestFn
to see if we can't make it more flexible, as well as more inline with the text and input style approach.
const requests = data.requests.filter( | ||
(request) => !networkOptions.ignoreRequestFn(request), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if this was both filtering and cleansing.
The most typical case for privacy controls is:
"I want to filter out requests to this place"
"I wan't to ensure certain headers aren't included"
"I want to filter out PII from the response bodies".
This would all doable with a slight modification of the function:
const requests = data.requests.filter( | |
(request) => !networkOptions.ignoreRequestFn(request), | |
); | |
const requests = data.requests.reduce( | |
(list, request) => { | |
const maskedRequest = networkOptions.maskRequestFn(request) | |
return maskedRequest ? [...list, maskedRequest] : list | |
}, [] | |
); |
Which then allows the function to be something like
maskRequestFn: (request: NetworkRequest): NetworkRequest | undefined => {
if (request.url.contains("rrweb-collector-api.com") {
return
}
delete request.requestHeaders["Authorization"]
request.responseBody = maskTextFn(request.responseBody)
return request
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory this could be added in a follow up PR as well. Happy to do that if that is the decision.
if (attempt > 10) { | ||
throw new Error('Cannot find performance entry'); | ||
} | ||
const urlPerformanceEntries = win.performance.getEntriesByName( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding correctly I think we'd need to make this optional...
If people already have their own code (or another dependency) tracking performance entries they might be calling https://developer.mozilla.org/en-US/docs/Web/API/Performance/clearResourceTimings already so we'd have a race.
Since this usage comes after the response if someone is clearing the buffer as soon as they're able to then I guess that this code would lose the race.
I think it's cheap enough to have the observer running no matter what that you could have recordRequestPerformance
as a config option and return early here so the signature becomes Promise<PerformanceRequestTiming | null>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake - 🤯 I've just read back through this and seen that the thrown error (and any other error here) are captured and swallowed where the returned promise is handled.
My preference would be to return null so that the flow is more clear but at least the error here doesn't bubble out
I would love this feature! Just throwing in a 👍 for this. |
type NetworkRequest = { | ||
url: string; | ||
method?: string; | ||
initiatorType: InitiatorType; | ||
status?: number; | ||
startTime: number; | ||
endTime: number; | ||
requestHeaders?: Headers; | ||
requestBody?: Body; | ||
responseHeaders?: Headers; | ||
responseBody?: Body; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was experimenting with this again...
We already capture Network timing and the PerformanceEntry
spec uses name
where this NetworkRequest
timing uses url
It is more natural for the typing here to be something like
export type CapturedNetworkRequest = Omit<PerformanceEntry, 'toJSON'> & {
method?: string
initiatorType?: InitiatorType
status?: number
startTime?: number
endTime?: number
requestHeaders?: Headers
requestBody?: Body
responseHeaders?: Headers
responseBody?: Body
// was this captured before fetch/xhr could have been wrapped
isInitial?: boolean
}
That way you can extend this to capture PerformanceEntry
s and/or headers and/or body and only use one type
Hello, I have recently started using rrrweb-io via the cdn . It would be very helpful to know when this plugin will be available on the CDN link. |
@jlalmes @YunFeng0817 Seems like this PR was approved long ago, can you please share what is the current blocker for merging? Many people in this thread are interested in it, and seems like most suggestions could be handled in followup PRs. Thank you! |
I'll just re-up this comment #1105 (comment) It's easier for me if we have |
We have network payload capture in prod now based on this PR - thanks so much @jlalmes 🙌 You can see the code here https://github.com/PostHog/posthog-js/blob/main/src/loader-recorder-v2.ts We couldn't use it quite as it is... and I've had to make a couple of bugfixes since releasing it - mostly around processing bodies on unexpected types of network requests My plan is to let it settle in our product for a week or two. And then I'll open a PR based on this one that would let us inject the things we'd varied - which might also be things other people would want to vary... It won't be massively different - and of course the version that works for us might not be preferable generally |
@jlalmes @pauldambra I was running into an issue with some of my requests not being recorded with this plugin, after digging deeper, turns out in case of certain requests, if the server doesn't return a Timing-Allow-Origin response header, we wouldn't be able to track the performance metrics. Given this plugin throws an error when performance metrics are not available, these requests were not being recorded. While the performance metrics are not available, Other data on these requests are still available, so in my case I had to do something like this to still record these requests. getRequestPerformanceEntry(win, 'fetch', req.url, after, before)
.then((entry) => {
if (_isNull(entry)) {
const requests = prepareRequestWithoutPerformance(req, networkRequest)
cb({ requests })
} else {
const requests = prepareRequest(entry, req.method, res?.status, networkRequest)
cb({ requests })
} })
function prepareRequestWithoutPerformance(
req: Request,
networkRequest: Partial<CapturedNetworkRequest>,
): CapturedNetworkRequest[] {
const reqJson = {
url: req.url,
method: req.method,
requestHeaders: networkRequest.requestHeaders,
requestBody: networkRequest.requestBody,
responseHeaders: networkRequest.responseHeaders,
responseBody: networkRequest.responseBody,
}
return [reqJson]
} Is this expected? |
@pauldambra would you still be interested to open a PR based on the version you have running in production? FYI: #1033 just got finished which moves the all of the plugins to the |
Hi @jlalmes , When can we see this in rrweb package? it would be very powerful and helpful feature! |
@Juice10 definitely... our implementation has been pretty stable for a while now... it's just time that's stopping me 🙈 I think the "tricky" thing will be separating our config/mapping needs out of the plugin but that's pretty mechanical non-ground-breaking stuff |
Hello, is there a guide on how to download this plugin? |
I have figuret it, you have to clone this repo https://github.com/jlalmes/rrweb/tree/feat/network-plugin then ckeckout to the branch "feat/network-plugin" and run "yarn && yarn run build:all" the files needed will be in the directory packages/rrweb/dist |
Closes #552
This implementation records
fetch
andXMLHttpRequest
by patching their object & methods. We record document navigation usingPerformanceNavigationTiming
and we usePerformanceResourceTiming
for recording everything else (script, img, link etc.) viaPerformanceObserver
API.Recording
initiatorTypes
is an array specifying which request types to recordignoreRequestFn
is a function that allow you to block recording an event for a request (you don't want to create a loop recording the request you send to storerrweb
events on your server)recordHeaders
will record an request/response headers forfetch
andxmlhttprequest
recordBody
will record an request/response bodies forfetch
andxmlhttprequest
recordInitialRequests
will record an event for all requests prior torrweb.record()
being calledReplaying
onNetworkData
function.To-do