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

feat: capture network payloads (internal alpha) #886

Merged
merged 9 commits into from
Nov 13, 2023

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Nov 12, 2023

rrweb has network payloads queued up as a feature... but it's taking a while.

The easiest way to test it is to adopt it ourselves.

This adds a copy of the plugin proposed for rrweb, and uses it to wrap xhr and fetch.

We can match performance timings and these new NetworkRequests based on URL and timings

used by PostHog/posthog#18562

for now this can only be enabled via decide response, which allows header and body capture to be configured separately

TODO

  • this is quite a size bump should we be lazy loading this (and performance observer?)
  • what limits should we be applying that we'll regret if we don't?
    • limit on payload size?

Copy link

github-actions bot commented Nov 12, 2023

Size Change: +12.9 kB (+2%)

Total Size: 732 kB

Filename Size Change
dist/array.full.js 173 kB +1.1 kB (+1%)
dist/array.js 115 kB +1.09 kB (+1%)
dist/es.js 115 kB +1.09 kB (+1%)
dist/module.js 115 kB +1.09 kB (+1%)
dist/recorder-v2.js 104 kB +8.58 kB (+9%) 🔍
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12 kB
dist/recorder.js 58.3 kB
dist/surveys.js 39.8 kB

compressed-size-action

Comment on lines 24 to 25
import { getRecordNetworkPlugin } from './network/record'
import { buildNetworkRequestOptions } from './network/record/default-options'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move all of this stuff (aside from maybe some types) into the recorder loader?

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great and is very exciting.

Only thing I'm wondering is if we sadly need an additional remote config flag for this, otherwise we will immediately start capturing request and response data for all existing people who have perf enabled...

@pauldambra pauldambra marked this pull request as ready for review November 13, 2023 12:55
Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets goooo

@pauldambra pauldambra changed the title feat: capture network payloads feat: capture network payloads (internal alpha) Nov 13, 2023
@pauldambra pauldambra added the bump minor Bump minor version when this PR gets merged label Nov 13, 2023
@pauldambra pauldambra merged commit 80e45cb into master Nov 13, 2023
14 checks passed
@pauldambra pauldambra deleted the feat/capture-network-payloads branch November 13, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants