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

ref(replay): try 5s rage/dead click timeout for sentry orgs #77286

Closed
wants to merge 2 commits into from

Conversation

aliu39
Copy link
Member

@aliu39 aliu39 commented Sep 10, 2024

@aliu39 aliu39 requested a review from a team as a code owner September 10, 2024 22:09
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 10, 2024
@@ -396,6 +396,15 @@ def _handle_breadcrumb(
) -> ReplayActionsEventPayloadClick | None:

click = None
project = Project.objects.get(id=project_id)
dead_click_timeout = (
5000
Copy link
Member

Choose a reason for hiding this comment

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

you could also make this an option if you wanted to have it be more controllable. but this makes sense!

@aliu39 aliu39 requested a review from a team September 10, 2024 22:11
Copy link

codecov bot commented Sep 10, 2024

❌ 6 Tests Failed:

Tests completed Failed Passed Skipped
21529 6 21523 205
View the top 3 failed tests by shortest run time
tests.sentry.replays.unit.test_ingest_dom_index test_emit_click_negative_node_id
Stack Traces | 0.052s run time
#x1B[1m#x1B[.../replays/unit/test_ingest_dom_index.py#x1B[0m:1139: in test_emit_click_negative_node_id
    user_actions = get_user_actions(1, uuid.uuid4().hex, events, None)
#x1B[1m#x1B[.../usecases/ingest/dom_index.py#x1B[0m:188: in get_user_actions
    click = _handle_breadcrumb(event, project_id, replay_id, replay_event)
#x1B[1m#x1B[.../usecases/ingest/dom_index.py#x1B[0m:399: in _handle_breadcrumb
    project = Project.objects.get(id=project_id)
#x1B[1m#x1B[31m.venv/lib/python3.12.../db/models/manager.py#x1B[0m:87: in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
#x1B[1m#x1B[31m.venv/lib/python3.12.../db/models/query.py#x1B[0m:649: in get
    raise self.model.DoesNotExist(
#x1B[1m#x1B[31mE   sentry.models.project.Project.DoesNotExist: Project matching query does not exist.#x1B[0m
tests.sentry.replays.unit.test_ingest_dom_index test_get_user_actions_missing_node
Stack Traces | 0.052s run time
#x1B[1m#x1B[.../replays/unit/test_ingest_dom_index.py#x1B[0m:121: in test_get_user_actions_missing_node
    user_actions = get_user_actions(1, uuid.uuid4().hex, events, None)
#x1B[1m#x1B[.../usecases/ingest/dom_index.py#x1B[0m:188: in get_user_actions
    click = _handle_breadcrumb(event, project_id, replay_id, replay_event)
#x1B[1m#x1B[.../usecases/ingest/dom_index.py#x1B[0m:399: in _handle_breadcrumb
    project = Project.objects.get(id=project_id)
#x1B[1m#x1B[31m.venv/lib/python3.12.../db/models/manager.py#x1B[0m:87: in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
#x1B[1m#x1B[31m.venv/lib/python3.12.../db/models/query.py#x1B[0m:649: in get
    raise self.model.DoesNotExist(
#x1B[1m#x1B[31mE   sentry.models.project.Project.DoesNotExist: Project matching query does not exist.#x1B[0m
tests.sentry.replays.unit.test_ingest_dom_index test_get_user_actions_str_payload
Stack Traces | 0.052s run time
#x1B[1m#x1B[.../replays/unit/test_ingest_dom_index.py#x1B[0m:99: in test_get_user_actions_str_payload
    user_actions = get_user_actions(1, uuid.uuid4().hex, events, None)
#x1B[1m#x1B[.../usecases/ingest/dom_index.py#x1B[0m:188: in get_user_actions
    click = _handle_breadcrumb(event, project_id, replay_id, replay_event)
#x1B[1m#x1B[.../usecases/ingest/dom_index.py#x1B[0m:399: in _handle_breadcrumb
    project = Project.objects.get(id=project_id)
#x1B[1m#x1B[31m.venv/lib/python3.12.../db/models/manager.py#x1B[0m:87: in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
#x1B[1m#x1B[31m.venv/lib/python3.12.../db/models/query.py#x1B[0m:649: in get
    raise self.model.DoesNotExist(
#x1B[1m#x1B[31mE   sentry.models.project.Project.DoesNotExist: Project matching query does not exist.#x1B[0m

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@@ -396,6 +396,15 @@ def _handle_breadcrumb(
) -> ReplayActionsEventPayloadClick | None:

click = None
project = Project.objects.get(id=project_id)
Copy link
Member

@cmanallen cmanallen Sep 11, 2024

Choose a reason for hiding this comment

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

This will blow up the database. You're running the query for every segment ingested (2k RPS) multiplied by every breadcrumb in the recording data. This is going to query the database anywhere from 20k times per second to 200k times per second. Or more realistically, we won't query at that rate and will begin to backlog as the database becomes the primary choke point. This approach won't work.

5000
if features.has(
"organizations:session-replay-dead-click-reduced-timeout",
project.organization,
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use a feature flag here. Let's set a list-type option. In options-automator you can specify the org-ids/project-ids you want to allow to use the new timeout. You can do the same for the timeout itself as mentioned by Josh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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