Skip to content

feat(replays): Add dead click issue detector #50149

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

Merged
merged 29 commits into from
Jun 5, 2023

Conversation

cmanallen
Copy link
Member

@cmanallen cmanallen commented Jun 1, 2023

Adds experimental support for creating Replay issues on the issues-platform. The first issue type is "slow click" and will be available for Sentry org only. Slow clicks issues are only created for timeout endReasons on a and button tags.

Depends on: #50148
Related: #48259

@cmanallen cmanallen requested a review from a team as a code owner June 1, 2023 13:01
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 1, 2023
@cmanallen cmanallen requested a review from a team as a code owner June 1, 2023 14:07
@cmanallen cmanallen changed the base branch from master to cmanallen/issues-add-replay-issue June 1, 2023 14:07
@cmanallen cmanallen requested a review from JoshFerge June 1, 2023 14:59
extra_event_data["event_id"] = event_id

occurrence = IssueOccurrence(
id=uuid.uuid4().hex,
Copy link
Member

Choose a reason for hiding this comment

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

what's the difference between id and event_id -- should either of them have some kind of deterministic value based on the event, or is random ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure random is okay. event_id and event["event_id"] should match but otherwise I think id can be whatever.

@wedamija Is this correct?

"id": "1",
"username": "Test User",
"email": "[email protected]",
},
Copy link
Member

Choose a reason for hiding this comment

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

Do we have access to all of the event schema here? If we do, we'd want to add also (in addition to @JoshFerge 's comment above):

"contexts": {
  "browser": ..
  "os": ..
  "device": ..
},
"breadcrumb": [..] 

The last 100 crumbs from the replay up to the timestamp of the triggered deadclick (not the click itself but including the timeout).

Wrt to event.sdk, do we report the value of the SDK that generated the click? Could be useful to track what data we have available and fixes to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Until #50157 is merged I won't have that data but that's a good point that we should add this metadata.

wrt to breadcrumbs. Whats the formatting of these values?

Copy link
Member

@JoshFerge JoshFerge Jun 1, 2023

Choose a reason for hiding this comment

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

@bruno-garcia we don't have any of the context in this payload at the moment, i am working on getsentry/relay#2170 (from getsentry/rfcs#82) to fulfill this along with Colton's above PR.

I don't think we will be able to construct the breadcrumbs easily in the consumer here at the moment, its worth thinking about for replay issues if the issue breadcrumb should just query the replay recording directly for them.


_report_slow_click_issue(
environment="prod",
fingerprint=payload["message"],
Copy link
Member

Choose a reason for hiding this comment

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

what is message here? is that the path of the HTML element?

Copy link
Member Author

@cmanallen cmanallen Jun 1, 2023

Choose a reason for hiding this comment

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

Yes, sentry's version of a CSS selector.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder how this will end up with duplicate / similar issues / new issues created when small changes are made to the DOM. something to think about for future iterations.

Copy link
Member

Choose a reason for hiding this comment

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

overall this looks good to me. I am curious if this will create hundreds of issues upon deployment -- I believe with the issue platform there is a way to throttle this. Also, I think we should try on the ET org before the Sentry org, and roll it out slowly on the Sentry org.

#50149 (comment)

Do we know the number of different messages we have already based on our tests? That'll be the number of issues we'll get

Copy link
Member

Choose a reason for hiding this comment

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

I believe @billyvg had a query for this?

Copy link
Member

@billyvg billyvg Jun 1, 2023

Choose a reason for hiding this comment

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

1324 total deadclick rows, 518 rows when grouped by message (over unknown timeperiod (this is from bigquery))

84 of the 518 have > 1 occurrence

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I'm filtering we should create roughly ~300 issues in 3 weeks. I'm not sure if those issues will come at all once or if they will come gradually over that time period.

Copy link
Member

Choose a reason for hiding this comment

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

it might be worth doing a bit of manual prodding of the particular issues now to see if we can find ones that are similar and come up with heuristics to see if we can combine them.

Copy link
Member

Choose a reason for hiding this comment

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

and our "similarity" implementation for stacktrace issues for inspiration:

similar items and estimate the Jaccard similarity of their characteristic

Copy link
Member

Choose a reason for hiding this comment

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

The way I'm filtering we should create roughly ~300 issues in 3 weeks.

If that means we have 300 different broken buttons and anchors I think that's fine. Otherwise worth digging further

Copy link
Member

@JoshFerge JoshFerge left a comment

Choose a reason for hiding this comment

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

overall this looks good to me. I am curious if this will create hundreds of issues upon deployment -- I believe with the issue platform there is a way to throttle this. Also, I think we should try on the ET org before the Sentry org, and roll it out slowly on the Sentry org.

@asottile-sentry
Copy link
Member

Just to clarify I did not request reviews from all of these teams...

yeah you've got a merge / rebase that's gone wrong -- effectively making it so you're doing the last N changes to master

@cmanallen cmanallen changed the base branch from cmanallen/issues-add-replay-issue to master June 2, 2023 13:35
@cmanallen cmanallen changed the title feat(replays): Add slow click issue detector feat(replays): Add dead click issue detector Jun 2, 2023
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #50149 (c9c3370) into master (31445be) will increase coverage by 0.00%.
The diff coverage is 95.34%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #50149   +/-   ##
=======================================
  Coverage   81.11%   81.12%           
=======================================
  Files        4843     4846    +3     
  Lines      203288   203326   +38     
  Branches    11253    11253           
=======================================
+ Hits       164904   164944   +40     
+ Misses      38135    38133    -2     
  Partials      249      249           
Impacted Files Coverage Δ
src/sentry/replays/usecases/ingest/dom_index.py 86.66% <60.00%> (+2.22%) ⬆️
src/sentry/replays/usecases/ingest/dead_click.py 100.00% <100.00%> (ø)
src/sentry/replays/usecases/ingest/events.py 100.00% <100.00%> (ø)
src/sentry/replays/usecases/issue.py 100.00% <100.00%> (ø)

@cmanallen cmanallen merged commit 23252ef into master Jun 5, 2023
@cmanallen cmanallen deleted the cmanallen/replays-add-slow-click-detector branch June 5, 2023 13:14
jan-auer added a commit that referenced this pull request Jun 5, 2023
* master: (52 commits)
  feat(replays): Add dead click issue detector (#50149)
  fix(sourcemaps): Improve metrics collection (#50293)
  ref(dynamic-sampling): Implement new abstraction for models (#50227)
  Revert "feat(backpressure): Proof of concept for queue monitoring service (#49872)"
  fix(dynamic-sampling): Remove duplicated queue declaration (#50283)
  feat(backpressure): Proof of concept for queue monitoring service (#49872)
  feat: Add pages for org auth tokens (#50225)
  feat(strings): Add span.category tag (#50286)
  feat(profiles): Count processed profiles in billing metrics consumer (#50047)
  feat(strings): Add `transaction.method` tag (#50237)
  ref(txnames): Increase sample size (#50278)
  ref(framework-suggestion-modal): Sort platforms by popularity (#50235)
  ref(hc): Only set the organizationmembermapping.member_id for monolith (#49789)
  ref(hc): Region name is set for organization mappings (#50257)
  fix(alert-details): Prevent environments call from being cancelled (#50270)
  fix(post-process-group): retry retrieving events from eventstore up to 3 times (#50210)
  fix(functions): Ensure selected functions projects are returned (#50194)
  styles(functions): Handle function name overflow in suspect functions (#50251)
  feat(functions): Add tooltip to slowest functions score (#50167)
  ref(routes): Avoid named exports for routes for now (#50255)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 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
Development

Successfully merging this pull request may close these issues.

5 participants