-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
Log query parameters on HTTP requests #3591
Log query parameters on HTTP requests #3591
Conversation
Follow-up to #3485
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.
One minor improvement, but looks good (and safe) overall, thanks!
Btw the Cypress tests are failing for unrelated reasons, so happy to merge when you're ready (ping me). |
@@ -300,7 +300,7 @@ describe("FetchHttpApi", () => { | |||
const fetchFn = jest.fn().mockReturnValue(deferred.promise); |
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.
happy to merge when you're ready (ping me).
@andybalaam Feel free to merge whenever 🙂
Thank you! |
This fell out of the merge queue because of known flake element-hq/element-web#25625. Re-adding. |
Thanks for the review @andybalaam 🐰 |
Log query parameters on HTTP requests
Follow-up to #3485
Before:
After:
All query parameters values are sanitized to
xxx
but the existence of the query parameter is helpful to distinguish between requests and a peace of mind sanity check to see that they are on the request. Like initial vs incremental/sync
with?since=xxx
. In the future, it would be nice to log more values.Dev notes
Checklist
Sign-off given on the changes (see CONTRIBUTING.md)This change is marked as an internal change (Task), so will not be included in the changelog.