feat: Update Group.filter_by_event_id to use Snuba by lynnagara · Pull Request #14035 · getsentry/sentry

lynnagara · 2019-07-16T17:52:51Z

This is the second part of #13905, which updates the group model to use events from Snuba instead of Postgres. This PR refactors the filter_by_event_id function.

fpacifici

A few questions but overall it seems ok

fpacifici · 2019-07-16T22:27:36Z

src/sentry/models/group.py

-            )
+        from sentry.utils import snuba
+
+        group_ids = set([evt['issue'] for evt in snuba.raw_query(


nit, I think this code would be more readable without the list comprehension.

fpacifici · 2019-07-16T22:30:30Z

src/sentry/models/group.py

+        from sentry.utils import snuba
+
+        group_ids = set([evt['issue'] for evt in snuba.raw_query(
+            start=datetime.utcfromtimestamp(0),


Is the retention policy we have in snuba the same as the one we have in Event and EventMapping table? Snuba will apply can apply a default maximum timespan.
https://github.com/getsentry/snuba/blob/master/snuba/api.py#L170

@JTCunning Do we use that setting in production?

We do have this setting in Production, but this should be scoped to an organization's retention.

You can find other queries scoped to retention like so:

sentry/src/sentry/search/snuba/backend.py

Lines 275 to 285 in 0a8c456

retention = quotas.get_event_retention(organization=projects[0].organization)

if retention:

retention_window_start = timezone.now() - timedelta(days=retention)

else:

retention_window_start = None

# TODO: This could be optimized when building querysets to identify

# criteria that are logically impossible (e.g. if the upper bound

# for last seen is before the retention window starts, no results

# exist.)

if retention_window_start:

group_queryset = group_queryset.filter(last_seen__gte=retention_window_start)

This also has lead me to believe that we have most likely not properly scoped other SnubaEvent-based queries to organization retention.

As discussed on Slack, Sentry's snuba client shrinks every time window to be inside retention.

sentry/src/sentry/utils/snuba.py

Lines 561 to 574 in 576048a

# any project will do, as they should all be from the same organization

project = Project.objects.get(pk=project_ids[0])

retention = quotas.get_event_retention(

organization=Organization(project.organization_id)

)

if retention:

start = max(start, datetime.utcnow() - timedelta(days=retention))

if start > end:

raise QueryOutsideRetentionError

# if `shrink_time_window` pushed `start` after `end` it means the user queried

# a Group for T1 to T2 when the group was only active for T3 to T4, so the query

# wouldn't return any results anyway

new_start = shrink_time_window(query_params.filter_keys.get('issue'), start)

Which goes back to my original question.
Are we going to return fewer results than in the previous implementation since there is no retention applied to the postgres query ?

No. Those are deleted at 90d every 5 minutes, and if we weren't previously capping the 30d retention, now is the time to.

fpacifici · 2019-07-16T22:33:56Z

src/sentry/models/group.py

+                'event_id': [event_id],
+                'project_id': project_ids,
+            },
+            limit=1000,


Can this query ever return more than 1 value? We may be able to make it more efficient for clickhouse if we could limit the results.

Theoretically I think it can since event IDs are only unique by project, not org. We only use this code here to detect a direct hit https://github.com/getsentry/sentry/blob/master/src/sentry/api/endpoints/organization_group_index.py#L129 and we end up discarding all other results apart from the first one anyway. I think it would be better to just return a single result here for direct hit, but not sure if we'll want to tackle that here.

~~I've updated this function to only look for a single direct hit, since that's the only place where we actually use this code.~~ As discussed on Slack, we need to preserve the existing behavior since event IDs are only unique by project. Updated limit to len(project_ids)

This is the second part of #13905, which updates the group model to use events from Snuba instead of Postgres. This PR updates the filter_by_event_id function.

lynnagara requested a review from a team July 16, 2019 17:52

lynnagara mentioned this pull request Jul 16, 2019

feat: Use Snuba events in group model #13905

Closed

lynnagara changed the base branch from ref/group-model-1 to master July 16, 2019 17:54

lynnagara changed the base branch from master to ref/group-model-1 July 16, 2019 17:55

fpacifici reviewed Jul 16, 2019

View reviewed changes

lynnagara changed the base branch from ref/group-model-1 to master July 17, 2019 18:17

lynnagara changed the title ~~feat: Use Snuba for Group.filter_by_event_id~~ feat: Update group direct hit behavior to use Snuba Jul 17, 2019

lynnagara added 2 commits July 17, 2019 13:06

feat: Use Snuba for Group.filter_by_event_id

864769f

This is the second part of #13905, which updates the group model to use events from Snuba instead of Postgres. This PR updates the filter_by_event_id function.

update limit

e9403d9

lynnagara force-pushed the feat/group-model-2 branch from 99a56f9 to e9403d9 Compare July 17, 2019 22:58

improve readability

4a93911

lynnagara changed the title ~~feat: Update group direct hit behavior to use Snuba~~ feat: Update Group.filter_by_event_id to use Snuba Jul 18, 2019

fpacifici approved these changes Jul 19, 2019

View reviewed changes

lynnagara merged commit 076da73 into master Jul 19, 2019

lynnagara deleted the feat/group-model-2 branch July 19, 2019 18:31

github-actions bot locked and limited conversation to collaborators Dec 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Update Group.filter_by_event_id to use Snuba#14035

feat: Update Group.filter_by_event_id to use Snuba#14035
lynnagara merged 3 commits intomasterfrom
feat/group-model-2

lynnagara commented Jul 16, 2019 •

edited

Loading

Uh oh!

fpacifici left a comment

Uh oh!

fpacifici Jul 16, 2019

Uh oh!

fpacifici Jul 16, 2019

Uh oh!

JTCunning Jul 16, 2019

Uh oh!

JTCunning Jul 17, 2019

Uh oh!

fpacifici Jul 18, 2019

Uh oh!

JTCunning Jul 18, 2019

Uh oh!

fpacifici Jul 16, 2019 •

edited

Loading

Uh oh!

lynnagara Jul 17, 2019

Uh oh!

lynnagara Jul 17, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	retention = quotas.get_event_retention(organization=projects[0].organization)
	if retention:
	retention_window_start = timezone.now() - timedelta(days=retention)
	else:
	retention_window_start = None
	# TODO: This could be optimized when building querysets to identify
	# criteria that are logically impossible (e.g. if the upper bound
	# for last seen is before the retention window starts, no results
	# exist.)
	if retention_window_start:
	group_queryset = group_queryset.filter(last_seen__gte=retention_window_start)

	# any project will do, as they should all be from the same organization
	project = Project.objects.get(pk=project_ids[0])
	retention = quotas.get_event_retention(
	organization=Organization(project.organization_id)
	)
	if retention:
	start = max(start, datetime.utcnow() - timedelta(days=retention))
	if start > end:
	raise QueryOutsideRetentionError

	# if `shrink_time_window` pushed `start` after `end` it means the user queried
	# a Group for T1 to T2 when the group was only active for T3 to T4, so the query
	# wouldn't return any results anyway
	new_start = shrink_time_window(query_params.filter_keys.get('issue'), start)

Uh oh!

Conversation

lynnagara commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fpacifici left a comment

Choose a reason for hiding this comment

Uh oh!

fpacifici Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

fpacifici Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

JTCunning Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

JTCunning Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

fpacifici Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

JTCunning Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

fpacifici Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynnagara Jul 17, 2019

Choose a reason for hiding this comment

Uh oh!

lynnagara Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lynnagara commented Jul 16, 2019 •

edited

Loading

fpacifici Jul 16, 2019 •

edited

Loading

lynnagara Jul 17, 2019 •

edited

Loading