-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(replay): Allow to configure URLs to capture network bodies/headers #7953
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
Conversation
size-limit report 📦
|
captureNetworkBodies: true, | ||
}, | ||
|
||
networkDetailAllowUrls: ['http://localhost:7654/foo'], |
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.
Just double checking, is the port here stable?
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.
🤷 this is also in the tests, so it would fail there as well (we check the url of the events is this in there)
@@ -78,33 +80,37 @@ async function _prepareFetchData( | |||
const { | |||
url, | |||
method, | |||
status_code: statusCode, | |||
status_code: statusCode = 0, |
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.
Should this default to 0
? Does undefined
vs explicit 0
have different meanings?
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.
We've been doing this before as well (only setting it further down) - but 🤷 I'd say it's fine?
/** | ||
* Capture the following request headers, in addition to the default ones. | ||
* Only applies to URLs matched by `networkDetailAllowUrls`. | ||
* Any headers defined here will be captured in addition to the default headers. |
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.
Should we list the default headers here?
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'd just document them in the docs, IMHO should be good enough? As this may change, I guess.
* Only applies to URLs matched by `networkDetailAllowUrls`. | ||
* Any headers defined here will be captured in addition to the default headers. | ||
*/ | ||
networkRequestHeaders: string[]; |
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.
Wonder if headers should be RegExp + string as well?
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's a valid point, although I think this is less of a use case probably. IMHO I'd leave this like this for now, we can always extend this later if we want to also allow regex.
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.
Yeah we can add it later -- I was testing this locally for sentry and found myself wanting to add x-sentry-*
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 think we need to also have a |
Good point! I think I can add this in a follow up PR, though. |
…IPPED` warning This actually does not exist in our SDK, instead we can make the same assumption using sdk configuration options (e.g. if they have the option enabled) and if the body is empty to determine if we should show the onboarding setup. ref: getsentry/sentry-javascript#7953 (comment) closes: getsentry/sentry-javascript#8004
Doing some tidying, we did not add a |
…IPPED` warning (#84654) This actually does not exist in our SDK, instead we can make the same assumption using sdk configuration options (e.g. if they have the option enabled) and if the body is empty to determine if we should show the onboarding setup. ref: getsentry/sentry-javascript#7953 (comment) closes: getsentry/sentry-javascript#8004
This replaces the old experimental options for enhanced replay network capture with new, official configuration:
Note: This brings a small change, in that we also just capture the default headers when the URL matches. So by default, no headers are captured for any request.
Example usage:
Capture bodies & default headers for given URL:
Capture bodies & default headers for given exact URLs:
Capture only headers, including some custom ones:
Warnings
For any request that does not match the given URL(s), we will add a warning to the request/response meta. For example:
If this is not present, and no body/headers are sent, it means that the request actually had no body/headers. We can use this in the UI to show appropriate hints to the user.
Dropped experiment
This drops the related experimental options.
Closes #7830