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(inbound-filters): Add react-hydration-errors filter #45188

Merged
merged 10 commits into from
Mar 3, 2023

Conversation

priscilawebdev
Copy link
Member

@priscilawebdev priscilawebdev commented Feb 28, 2023

Problem:

We want to make it clear how Sentry is transforming error payloads, specifically for react-minified errors. This is because it can be hard to tell how to filter these errors (using either beforeSend or server-side inbound filters) as the UI shows the transformed message, not the original one.

Solution:

A new "Inbound Filters" called "Filter out hydration errors" is added via this PR, so users only have to toggle and relay will filter out the most common react hydration errors.

More Details:
Fixes: #45038
Fixes: getsentry/sentry-javascript#6295

@@ -0,0 +1,3 @@
{"event_id":"65fad5091a3f4db1b9812777f6dcc052","sent_at":"2023-02-28T16:05:58.058Z","sdk":{"name":"sentry.javascript.nextjs","version":"7.39.0"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep... I think it was included by mistake @priscilawebdev ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes that was included by mistake... I am removing and turning this back to "ready to review" 😉

Copy link
Contributor

@RaduW RaduW left a comment

Choose a reason for hiding this comment

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

With the exception of the Json file (which I think was added by mistake) everything seems fine.
Please remove the test.json (unless I completely miss something) then I think it's good to go.

@@ -14,6 +14,12 @@ source: tests/sentry/api/endpoints/test_project_filters.py
hello: localhost - Filter out events coming from localhost
id: localhost
name: Filter out events coming from localhost
- active: false
Copy link
Member Author

Choose a reason for hiding this comment

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

@andriisoldatenko @RaduW I used the following command to update this file 😉

SENTRY_SNAPSHOTS_WRITEBACK=1 pytest tests/sentry/api/endpoints/test_project_filters.py

Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

(I've only looked at the relay part.) Does this come with a PR in relay?

Comment on lines 238 to 241
_react_hydration_errors_filter = _FilterSpec(
id=FilterStatKeys.REACT_HYDRATION_ERRORS,
name="Filter out hydration errors",
description="React falls back to do a full re-render on a page and these errors are often not actionable.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we targeting all hydration errors, or only these coming from React?

# Text content does not match server-rendered HTML.
"https://reactjs.org/docs/error-decoder.html?invariant=425",
]
error_messages = [*error_messages, *react_known_hydration_errors]
Copy link
Contributor

Choose a reason for hiding this comment

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

error_messages here may be None if the option above is not registered, and in that case the unpacking fails.

"https://reactjs.org/docs/error-decoder.html?invariant=425",
]
error_messages = [*error_messages, *react_known_hydration_errors]

if error_messages:
filter_settings["errorMessages"] = {"patterns": error_messages}
Copy link
Contributor

Choose a reason for hiding this comment

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

errorMessages are here in relay, and patterns are globs. Do you want to escape special characters in the links above, like ??

@priscilawebdev priscilawebdev requested a review from a team as a code owner March 1, 2023 11:43
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

🚨 Warning: This pull request contains Frontend and Backend changes!

It's discouraged to make changes to Sentry's Frontend and Backend in a single pull request. The Frontend and Backend are not atomically deployed. If the changes are interdependent of each other, they must be separated into two pull requests and be made forward or backwards compatible, such that the Backend or Frontend can be safely deployed independently.

Have questions? Please ask in the #discuss-dev-infra channel.

@priscilawebdev priscilawebdev force-pushed the feat/add-react-hydration-errors-inbound-filter branch 2 times, most recently from c42f48b to deacd7d Compare March 1, 2023 11:48
@priscilawebdev
Copy link
Member Author

priscilawebdev commented Mar 1, 2023

Sorry @iker-barriocanal but I just completely changed the logic based on @jan-auer feedback

@priscilawebdev priscilawebdev requested a review from jan-auer March 1, 2023 11:50
@codecov
Copy link

codecov bot commented Mar 2, 2023

Codecov Report

Merging #45188 (2413734) into master (043f4ac) will decrease coverage by 2.69%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #45188      +/-   ##
==========================================
- Coverage   79.71%   77.02%   -2.69%     
==========================================
  Files        4724     4720       -4     
  Lines      198812   198690     -122     
  Branches    12007    12006       -1     
==========================================
- Hits       158482   153050    -5432     
- Misses      40068    45378    +5310     
  Partials      262      262              
Impacted Files Coverage Δ
src/sentry/api/serializers/models/project.py 97.16% <ø> (-0.26%) ⬇️
src/sentry/models/options/project_option.py 98.64% <ø> (ø)
...pp/components/events/eventReplay/replayPreview.tsx 96.42% <ø> (-0.13%) ⬇️
...ponents/events/interfaces/frame/stacktraceLink.tsx 85.07% <ø> (-0.85%) ⬇️
static/app/components/menuListItem.tsx 100.00% <ø> (ø)
static/app/components/replays/playerDOMAlert.tsx 100.00% <ø> (ø)
...tics/integrations/stacktraceLinkAnalyticsEvents.ts 100.00% <ø> (ø)
static/app/views/issueDetails/groupDetails.tsx 64.28% <ø> (ø)
static/app/views/monitors/monitors.tsx 0.00% <ø> (ø)
...c/app/views/organizationStats/usageChart/index.tsx 73.91% <ø> (ø)
... and 475 more

@priscilawebdev priscilawebdev force-pushed the feat/add-react-hydration-errors-inbound-filter branch 2 times, most recently from 4a6c887 to 167ee74 Compare March 2, 2023 10:06
@priscilawebdev
Copy link
Member Author

frontend #45300

@priscilawebdev priscilawebdev removed the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 2, 2023
@priscilawebdev priscilawebdev enabled auto-merge (squash) March 2, 2023 10:08
@priscilawebdev priscilawebdev disabled auto-merge March 2, 2023 10:17
Comment on lines 124 to 132
"https://reactjs.org/docs/error-decoder.html?invariant=418",
# The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.
"https://reactjs.org/docs/error-decoder.html?invariant=419",
# There was an error while hydrating this Suspense boundary. Switched to client rendering.
"https://reactjs.org/docs/error-decoder.html?invariant=422",
# There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.
"https://reactjs.org/docs/error-decoder.html?invariant=423",
# Text content does not match server-rendered HTML.
"https://reactjs.org/docs/error-decoder.html?invariant=425",
Copy link
Member

Choose a reason for hiding this comment

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

Because error_messages parses glob patterns, we could shorten this to

Suggested change
"https://reactjs.org/docs/error-decoder.html?invariant=418",
# The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.
"https://reactjs.org/docs/error-decoder.html?invariant=419",
# There was an error while hydrating this Suspense boundary. Switched to client rendering.
"https://reactjs.org/docs/error-decoder.html?invariant=422",
# There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.
"https://reactjs.org/docs/error-decoder.html?invariant=423",
# Text content does not match server-rendered HTML.
"https://reactjs.org/docs/error-decoder.html?invariant=425",
"https://reactjs.org/docs/error-decoder.html?invariant={418,419,422,423,425}"

Copy link
Member

@HazAT HazAT Mar 2, 2023

Choose a reason for hiding this comment

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

Let's please leave it like this, obviously more verbose, but the comments are helpful to understand what the numbers actually mean.

Copy link
Member

Choose a reason for hiding this comment

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

I'm totally in favor of keeping the comments, but we could still shorten the value itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I have updated the code shortening the value but still keeping the comments... Hope it's better now

jan-auer and others added 2 commits March 2, 2023 11:26
Adds an ingest consumer similar to Replays and Profiles that reads from
a Kafka topic `ingest-monitors` and updates them directly in postgres.
The consumer runs automatically in devserver and can be started via
`sentry run ingest-monitors`.

This supports creating and updating check ins with a status and
duration. Creating monitors is not supported yet. The consumer closely
follows the logic from checkin creation, but there is a difference for
disabled monitors compared to the Update (PUT) endpoint.
@priscilawebdev priscilawebdev merged commit dfe56ae into master Mar 3, 2023
@priscilawebdev priscilawebdev deleted the feat/add-react-hydration-errors-inbound-filter branch March 3, 2023 08:07
@github-actions github-actions bot locked and limited conversation to collaborators Mar 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
7 participants