feat: Use Snuba events in group model by lynnagara · Pull Request #13905 · getsentry/sentry

lynnagara · 2019-07-05T21:00:52Z

Refactor group model to always use events from Snuba instead of Postgres.
Rewrite a bunch of tests to use store_event instead of create_event as
this writes the events into Snuba.

Refactor group model to always use events from Snuba instead of Postgres

markstory · 2019-07-08T14:42:58Z

tests/sentry/api/endpoints/test_group_integration_details.py

                    }, {
                        'default': ('Sentry Issue: [%s](%s)\n\n```\n'
-                                    'Stacktrace (most recent call last):\n\n  '
+                                    'Stacktrace (most recent call first):\n\n  '


Why does this change?

I'm not actually sure, @tkaemming - do you know?

Hm... seems like that comes from here. Seems like the changes from create_group/create_event maybe triggered something like the platform to be saved differently? (I don't know if that's the case, I didn't follow the fixture creation down to the source.)

markstory · 2019-07-08T14:45:39Z

src/sentry/models/group.py

            issue_id=self.id,
            project_id=self.project_id)

-    def get_oldest_event(self):


Could plugins in sentry-plugins or other plugin repos be using this?

Good point. I checked the whole Sentry org on github and there doesn't seem to be any other usages of this

fpacifici · 2019-07-09T00:16:04Z

src/sentry/models/group.py

-        # Look up event_id in both Event and EventMapping,
-        # and bail when it matches one of them, prioritizing
-        # Event since it contains more history.
-        for model in Event, EventMapping:


I don't think we can remove the EventMapping check yet.
EventMapping is saved if the event is sampled (see

sentry/src/sentry/event_manager.py

Lines 717 to 731 in 618bb0c

if is_sample:

try:

with transaction.atomic(using=router.db_for_write(EventMapping)):

EventMapping.objects.create(project=project, group=group, event_id=event_id)

except IntegrityError:

logger.info(

'duplicate.found',

exc_info=True,

extra={

'event_uuid': event_id,

'project_id': project.id,

'group_id': group.id if group else None,

'model': EventMapping.__name__,

}

)

)

Even after removing events from postgres, eventmapping is going to stay.

Same as below, won't all the events be in Snuba?

fpacifici · 2019-07-09T00:23:01Z

src/sentry/models/group.py

-                    break
-            except IndexError:
-                pass
+        event = SnubaEvent.objects.from_event_id(event_id, project.id)


Before merging this, could you please check who is using the method and ensure we won't increase the load on snuba too much. Which means ensure this does not happen in the ingestion flow.

This one seems fine

Could you please provide some details about the research you have done ? What did you look into ?

Yeah sorry, this function seems to be just used in these 3 places, none of them are related to ingestion

src/sentry/web/frontend/error_page_embed.py: report.group = Group.objects.from_event_id(report.project, report.event_id) src/sentry/api/endpoints/project_group_index.py: matching_group = Group.objects.from_event_id(project, event_id) src/sentry/api/endpoints/project_user_reports.py: report.group = Group.objects.from_event_id(project, report.event_id)

fpacifici · 2019-07-09T00:24:45Z

src/sentry/models/group.py

        return Group.objects.get(id=group_id)

    def filter_by_event_id(self, project_ids, event_id):
-        from sentry.models import EventMapping, Event


Same issue, I think the check on EventMapping is still valid because it is the only way to fetch sampled events.

I was under the impression that events are not sampled in Snuba (unlike Postgres) so we would not need to additionally check EventMapping.. is that not correct?

Good quesiton. @tkaemming ?

This is correct, to my knowledge.

I guess if we're going to be introducing a hard dependency on Snuba for events we should probably just go ahead and get rid of all vestiges of sampling (incl. EventMapping.)

fpacifici · 2019-07-09T00:26:51Z

src/sentry/models/group.py

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


Any reason for this limit specifically. And same question as before. Please ensure we do not go through this code during event ingestion.

It's just some arbitrarily large number

fpacifici · 2019-07-09T00:39:38Z

src/sentry/models/group.py

@@ -390,21 +374,7 @@ def get_score(self):
        return type(self).calculate_score(self.times_seen, self.last_seen)

    def get_latest_event(self):


This may be a little tricker to remove.
I saw it is used in the plugin code:

src/sentry/plugins/base/v1.py: event = group.get_latest_event() or Event()
src/sentry/plugins/bases/issue.py: event = group.get_latest_event()
src/sentry/plugins/bases/issue2.py: event = group.get_latest_event()
src/sentry/plugins/bases/issue2.py: event = group.get_latest_event()
src/sentry/tasks/email.py: event = group.get_latest_event()

I can think about two possible issues:

load on snuba

the event may have not been stored in snuba while it would already been in postgres. postgres write happens before we execute plugins in post_process.py while at that point the event has just been produced in kafka. so it may or may not have been consumed yet. This should be verified.

Re 2: We might get away with this since these all seem to be view related functions - the event object is passed to any post process plugin code so we don't need to do .get_latest_event() there

Hmm, this might come up in slack unfurls

Actually post_process is synchronized on clickhouse storage. So it should be fine https://github.com/getsentry/sentry/blob/master/src/sentry/eventstream/kafka/consumer.py#L119-L145

fpacifici · 2019-07-09T00:45:14Z

src/sentry/models/group.py

    def get_latest_event(self):
-        from sentry.models import Event
-
-        if not hasattr(self, '_latest_event'):


We dropped this local cache here. Is this intentional ?

Hmm, I think this was an oversight, will bring this back

fpacifici

I'd suggest to split this PR to reduce the risk and make it easier to review considering the number of possible side effects this code can have.
You could take one method on the Group class per PR so you can focus on a reduced set of possible issues when monitoring production. Also this would make a revert easier in case something goes wrong.

fpacifici · 2019-07-16T00:39:27Z

src/sentry/models/group.py

-                    break
-            except IndexError:
-                pass
+        event = SnubaEvent.objects.from_event_id(event_id, project.id)


Could you please provide some details about the research you have done ? What did you look into ?

fpacifici · 2019-07-16T00:40:08Z

src/sentry/models/group.py

        return Group.objects.get(id=group_id)

    def filter_by_event_id(self, project_ids, event_id):
-        from sentry.models import EventMapping, Event


Good quesiton. @tkaemming ?

fpacifici · 2019-07-16T00:44:07Z

src/sentry/models/group.py

@@ -390,21 +374,7 @@ def get_score(self):
        return type(self).calculate_score(self.times_seen, self.last_seen)

    def get_latest_event(self):


Actually post_process is synchronized on clickhouse storage. So it should be fine https://github.com/getsentry/sentry/blob/master/src/sentry/eventstream/kafka/consumer.py#L119-L145

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

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 · 2019-07-16T17:53:43Z

Splitting this into a number of smaller PRs:

Part 3 of #13905

Removes the "snuba.events-queries.enabled" option and legacy code paths that were not being used in production. Also removes the unused Group.get_oldest_event method. Part 4 of #13905

This is the fifth (and final) part of #13905 Group model now uses Snuba Event instead of Postgres Event everywhere.

fpacifici · 2019-07-16T22:19:10Z

thanks for splitting it

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

Part 3 of #13905

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.

This is the fifth (and final) part of #13905 Group model now uses Snuba Event instead of Postgres Event everywhere.

Removes the "snuba.events-queries.enabled" option and legacy code paths that were not being used in production. Also removes the unused Group.get_oldest_event method. Part 4 of #13905

…14038) Removes the "snuba.events-queries.enabled" option and legacy code paths that were not being used in production. Also removes the unused Group.get_oldest_event method. Part 4 of #13905

This is the fifth (and final) part of #13905 Group model now uses Snuba Event instead of Postgres Event everywhere.

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.

This is the fifth (and final) part of #13905 Group model now uses Snuba Event instead of Postgres Event everywhere.

feat: Use Snuba events in group model

8b234c2

Refactor group model to always use events from Snuba instead of Postgres

lynnagara added the WIP label Jul 5, 2019

fix tests

338616d

lynnagara force-pushed the ref/group-model branch from 762f5a1 to 338616d Compare July 6, 2019 00:03

lynnagara added 2 commits July 5, 2019 17:37

fix jira tests

3746387

fix more tests

cb20a68

markstory reviewed Jul 8, 2019

View reviewed changes

fpacifici requested a review from tkaemming July 8, 2019 18:11

lynnagara force-pushed the ref/group-model branch from 5070c60 to d941aa6 Compare July 8, 2019 18:43

fix moar tests

3d3aa8b

lynnagara force-pushed the ref/group-model branch from d941aa6 to 3d3aa8b Compare July 8, 2019 19:36

various updates

d8409e0

fpacifici requested changes Jul 9, 2019

View reviewed changes

lynnagara added 2 commits July 9, 2019 10:22

fix lint error

b363bf4

bring back latest event cache

52f15da

lynnagara removed the WIP label Jul 9, 2019

lynnagara requested a review from fpacifici July 15, 2019 20:24

fpacifici requested changes Jul 16, 2019

View reviewed changes

lynnagara added a commit that referenced this pull request Jul 16, 2019

feat: Use Snuba for Group.from_event_id

9db7c31

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

lynnagara mentioned this pull request Jul 16, 2019

feat: Use Snuba for Group.from_event_id #14034

Merged

lynnagara added a commit that referenced this pull request Jul 16, 2019

feat: Use Snuba for Group.filter_by_event_id

1c006e5

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 added the Do Not Merge Don't merge label Jul 16, 2019

lynnagara mentioned this pull request Jul 16, 2019

feat: Update Group.filter_by_event_id to use Snuba #14035

Merged

lynnagara added a commit that referenced this pull request Jul 16, 2019

ref: Remove unused Group.event_set property

f53a9c9

Part 3 of #13905

lynnagara mentioned this pull request Jul 16, 2019

ref: Remove unused Group.event_set property #14036

Merged

lynnagara mentioned this pull request Jul 16, 2019

ref: Remove unused Group.get_oldest_event and legacy events behavior #14038

Merged

lynnagara added a commit that referenced this pull request Jul 16, 2019

feat: Update Group.get_latest_event to use Snuba event

a9d44c7

This is the fifth (and final) part of #13905 Group model now uses Snuba Event instead of Postgres Event everywhere.

lynnagara mentioned this pull request Jul 16, 2019

feat: Update Group.get_latest_event to use Snuba event #14039

Merged

lynnagara added a commit that referenced this pull request Jul 17, 2019

feat: Use Snuba for Group.from_event_id (#14034)

46961df

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

lynnagara added a commit that referenced this pull request Jul 17, 2019

ref: Remove unused Group.event_set property

7af0c0f

Part 3 of #13905

lynnagara added a commit that referenced this pull request Jul 17, 2019

ref: Remove unused Group.event_set property (#14036)

c3648be

Part 3 of #13905

lynnagara added a commit that referenced this pull request Jul 17, 2019

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.

lynnagara added a commit that referenced this pull request Jul 17, 2019

feat: Update Group.get_latest_event to use Snuba event

4c7fb63

This is the fifth (and final) part of #13905 Group model now uses Snuba Event instead of Postgres Event everywhere.

lynnagara added a commit that referenced this pull request Jul 18, 2019

feat: Update Group.get_latest_event to use Snuba event

3a1b342

This is the fifth (and final) part of #13905 Group model now uses Snuba Event instead of Postgres Event everywhere.

lynnagara closed this Jul 22, 2019

lynnagara deleted the ref/group-model branch July 22, 2019 18:42

lynnagara added a commit that referenced this pull request Jul 22, 2019

feat: Update Group.get_latest_event to use Snuba event (#14039)

9a3e5e3

This is the fifth (and final) part of #13905 Group model now uses Snuba Event instead of Postgres Event everywhere.

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

	if is_sample:
	try:
	with transaction.atomic(using=router.db_for_write(EventMapping)):
	EventMapping.objects.create(project=project, group=group, event_id=event_id)
	except IntegrityError:
	logger.info(
	'duplicate.found',
	exc_info=True,
	extra={
	'event_uuid': event_id,
	'project_id': project.id,
	'group_id': group.id if group else None,
	'model': EventMapping.__name__,
	}
	)

		@@ -390,21 +374,7 @@ def get_score(self):
		return type(self).calculate_score(self.times_seen, self.last_seen)

		def get_latest_event(self):

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkaemming Jul 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynnagara Jul 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpacifici left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

fpacifici commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lynnagara commented Jul 5, 2019 •

edited

Loading

tkaemming Jul 9, 2019 •

edited

Loading

lynnagara Jul 9, 2019 •

edited

Loading

lynnagara commented Jul 16, 2019 •

edited

Loading