perf(eventstore): get_events fetches event data from nodestore #16230

lynnagara · 2019-12-31T19:19:47Z

These queries yield 10+ columns from Snuba (including all
of the tag data) and are frequently taking a long time.
Instead we now fetch the minimal column set from Snuba, then use
those IDs to populate the event bodies from nodestore. Users of
eventstore.get_events() should no longer manually bind nodes since this
has already been done.

This depends on improvements to the message / search_message field #16473

These queries yield 10+ columns from Snuba (including all of the tag data) and are frequently timing out in production. Instead we now fetch the minimal column set from Snuba, then use those IDs to populate the event bodies from nodestore. Users of eventstore.get_events() should no longer manually bind nodes since this has already been done.

lynnagara · 2019-12-31T19:25:59Z

tests/sentry/eventstore/snuba_discover/test_backend.py


        self.eventstore = SnubaDiscoverEventStorage()

-    def test_get_events(self):


These were unnecessary duplicates of snuba/test_backend.py

lynnagara · 2019-12-31T19:28:49Z

tests/snuba/models/test_event.py

        assert event.data._node_data is not None
        assert event.data["user"]["id"] == u"user1"
-
-    def test_event_with_no_body(self):


Since we don't have sampling as a feature anymore, I don't think this is a valid use case. Much of our product already depends on the events being present in nodestore and would otherwise be breaking if the event bodies were missing.

This code didn't previously work (user should be an interface of an event), it just happens to not currently be called in any situation where snuba data isn't populated.

fpacifici · 2020-01-01T01:26:18Z

src/sentry/models/event.py

            return self.snuba_data["message"]
-        return self.data.get("message")
+        from sentry.event_manager import EventManager
+        event_manager = EventManager(self.data.data)


Isn't EventManager depending on Event itself ? Could we avoid this dependency by moving get_search_message ? search_message feels like a property of an event.

fpacifici · 2020-01-01T01:30:16Z

src/sentry/eventstore/snuba/backend.py


        if "error" not in result:
-            return [SnubaEvent(evt) for evt in result["data"]]
+            events = [SnubaEvent(evt) for evt in result["data"]]


Could you elaborate on how you noticed the performance issue? Knowing that the timeout is around 30 seconds, I would be surprised if the culprit was fetching multiple columns instead and I would be less surprised if the issue was the select process, which means the queries are still likely to timeout. Could this be related to the max_thread parameter?

I think this is more about increasing the speed of existing queries, than reducing the count of timed out queries (hopefully we don't have too many of those). I've seen a number of transactions that look similar to this https://sentry.io/organizations/sentry/eventsv2/snuba:7d55be78a00d43329937c42fcba9d2d5. The 2nd query is significantly slower than the first since it's getting all columns . I've changed the get_events implementation to only ever get the minimal columns from Snuba and the rest from Nodestore.

fpacifici · 2020-01-01T01:34:15Z

src/sentry/eventstore/snuba/backend.py

        if "error" not in result:
-            return [SnubaEvent(evt) for evt in result["data"]]
+            events = [SnubaEvent(evt) for evt in result["data"]]
+            self.bind_nodes(events)


This way we would be doing one request to bigtable per event. I guess, it would be more efficient to rely on nodestore.get_multi that makes one query and streams the results.
Also would the performance improvement be still relevant if we fetch like 1000 events (which means blasting bigtable with 1000 requests)

There were 2 place where we were previously fetching more than 100 events:

unmerge: This fetches 500 events at a time, since it was already doing bind_nodes immediately after the eventstore fetch (which has been removed) there should be no difference in performance

nodestore deletions: Rewrote to not use eventstore since we only need to get 2 columns

If we are doing a multi_get (which we are) I think we should be fine with that. But we should keep an eye on this.

The only place we are adding a multi get where where we weren't (at some later point outside eventstore) is in the group deletions task, we should keep an eye on that.

…odestore

fpacifici · 2020-01-07T01:18:35Z

src/sentry/eventstore/snuba/backend.py

        if "error" not in result:
-            return [SnubaEvent(evt) for evt in result["data"]]
+            events = [SnubaEvent(evt) for evt in result["data"]]
+            self.bind_nodes(events)


If we are doing a multi_get (which we are) I think we should be fine with that. But we should keep an eye on this.

fpacifici · 2020-01-07T01:37:20Z

src/sentry/api/endpoints/group_hashes.py

+        event = SnubaEvent(
+            {
+                "timestamp": result["latest_event_timestamp"],
+                "event_id": result["event_id"],
+                "group_id": group_id,
+                "project_id": project_id,
+            }
+        )
+        event.bind_node_data()


What does this become after SnubaEvent is gone ?

This has been fixed in #16286, will rebase to pull in those changes

fpacifici · 2020-01-07T01:41:10Z

src/sentry/deletions/defaults/group.py

            referrer="deletions.group",
            orderby=["-timestamp", "-event_id"],
-        )
+        )["data"]


I don't think we should bypass eventstore to fetch events. See my comment on PR #16284.

Let's first see if loading everything from eventstore is a problem. Maybe we are fine with a multi_get. If it is a problem we should revisit the issue of loading a small number of fields on a big number of events through eventstore.

I'll revert this commit, and we can observe if it proves to be a problem before attempting something like this.

This reverts commit 8c12bde.

…odestore

fpacifici

So after the discussion we had I believe you are going to still support binding nodes in some cases. RIght ?

…odestore

fpacifici

Failing tests are related.

fpacifici · 2020-01-15T00:47:06Z

src/sentry/eventstore/base.py

+    def get_unfetched_events(
+        self, filter, orderby=None, limit=100, offset=0, referrer="eventstore.get_unfetched_events"
+    ):
+        raise NotImplementedError


Docstring here is specifically important to explain the difference with get_events

fpacifici · 2020-01-15T00:52:25Z

src/sentry/eventstore/models.py

            return self._snuba_data[column]
        return super(Event, self).location

+    def get_search_message(self, event_metadata=None, culprit=None):


Is this change actually needed for fetching data from nodestore ?

It's a dependency. This was kind of a hack before since the message from snuba is different from the message from nodestore. We're no longer providing a way to explicitly say that you want the column to be loaded from snuba (a good thing since this was being abused before and fields should be the same regardless of the source they are loaded from). I'm going to try to improve this a bit more in a separate PR.

See #16473 for a more proper fix for the event.message / event.search_message behaviour

fpacifici · 2020-01-15T00:53:51Z

src/sentry/eventstore/snuba/backend.py

-        Get events from Snuba.
+        Get events from Snuba, with node data loaded.
        """
+        return self.get_events(


is this a call to __get_events instead ?

uhhh, yes it should be

…odestore

Since search_message is distinct from message, we should have them as separate properties of Event, rather than use them interchangably. Search_message is the internal only property for search, while message is the publicly exposed property. We temporarily save search_message into the message field in event_manager.save() to maintain backward compatibility with Django event format.

… fetch-events-from-nodestore

…odestore

This reverts commit 7ee79ad.

#16230)" This reverts commit 6635af5.

This reverts eventstore changes introduced in #16230, which bind node data. Since loading from nodestore is slower than expected we will attempt this change again once node data caching is introduced. I am not using reverting the entire commit since we still want to keep the search_message / message improvements that were included in the original.

lynnagara commented Dec 31, 2019

View reviewed changes

lynnagara requested a review from a team December 31, 2019 19:29

lynnagara changed the title ~~perf(eventstore): Fetch event data from nodestore~~ perf(eventstore): get_events fetches event data from nodestore Dec 31, 2019

fix(SnubaEvent): Fix bug in get_minimal_user

38ba9f5

This code didn't previously work (user should be an interface of an event), it just happens to not currently be called in any situation where snuba data isn't populated.

lynnagara mentioned this pull request Dec 31, 2019

fix(SnubaEvent): Fix bug in get_minimal_user #16231

Merged

update test

153e727

lynnagara force-pushed the fetch-events-from-nodestore branch from 627bdc0 to 153e727 Compare December 31, 2019 21:58

fix message

2b2b77f

lynnagara force-pushed the fetch-events-from-nodestore branch from ff86950 to 2b2b77f Compare December 31, 2019 22:16

pull in new store_event tests

377453a

lynnagara mentioned this pull request Jan 1, 2020

perf(eventstore): Fetch event from nodestore instead of Snuba #16210

Merged

fpacifici requested changes Jan 1, 2020

View reviewed changes

lynnagara added 2 commits January 3, 2020 13:42

revert message change

2946d0e

fix message

97882c0

lynnagara force-pushed the fetch-events-from-nodestore branch from 10f9d5a to 97882c0 Compare January 6, 2020 21:14

lynnagara added 3 commits January 6, 2020 13:36

remove unnecessary bind nodes

1c4db8d

Merge remote-tracking branch 'origin/master' into fetch-events-from-n…

4233e25

…odestore

don't use eventstore for deletions

8c12bde

fpacifici reviewed Jan 7, 2020

View reviewed changes

lynnagara added 5 commits January 7, 2020 11:21

Revert "don't use eventstore for deletions"

fd419f2

This reverts commit 8c12bde.

Merge remote-tracking branch 'origin/master' into fetch-events-from-n…

4a57d6b

…odestore

update test

d922f34

Merge remote-tracking branch 'origin/master' into fetch-events-from-n…

b6f4aee

…odestore

Merge remote-tracking branch 'origin/master' into fetch-events-from-n…

2da1212

…odestore

fpacifici requested changes Jan 13, 2020

View reviewed changes

lynnagara added 3 commits January 13, 2020 16:35

Merge remote-tracking branch 'origin/master' into fetch-events-from-n…

629c350

…odestore

make search_message a property of event

85fc876

update deletion to use new method

1750411

refactor

5f6fa3d

fpacifici requested changes Jan 15, 2020

View reviewed changes

lynnagara added 12 commits January 14, 2020 17:58

call the right method

3179174

docstrings

cffd347

revert raw_query

eb443ab

Merge remote-tracking branch 'origin/master' into fetch-events-from-n…

697ab55

…odestore

remove unnecessary import

b53501c

comment

156043a

update docstring

dd25797

Merge remote-tracking branch 'origin/event-message-improvements' into…

094c1cd

… fetch-events-from-nodestore

move search_message to EventCommon

d6f099a

Merge remote-tracking branch 'origin/event-message-improvements' into…

13037df

… fetch-events-from-nodestore

fix merge issue

2f8616c

fpacifici approved these changes Jan 15, 2020

View reviewed changes

lynnagara added 5 commits January 15, 2020 15:57

replace accidentally deleted function

7322c98

remove old stuff

c3ac9f2

Merge remote-tracking branch 'origin/master' into fetch-events-from-n…

391253e

…odestore

temporarily try running test suite with flag flipped

7ee79ad

Revert "temporarily try running test suite with flag flipped"

96e720b

This reverts commit 7ee79ad.

lynnagara merged commit 6635af5 into master Jan 16, 2020

lynnagara deleted the fetch-events-from-nodestore branch January 16, 2020 22:03

lynnagara mentioned this pull request Jan 17, 2020

ref: Make search_message a property of Event #16473

Closed

lynnagara added a commit that referenced this pull request Jan 17, 2020

Revert "perf(eventstore): get_events fetches event data from nodestore (

b3cdf6c

#16230)" This reverts commit 6635af5.

lynnagara mentioned this pull request Jan 17, 2020

fix(eventstore): Revert Nodestore changes in get_events #16518

Merged

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


		self.eventstore = SnubaDiscoverEventStorage()

		def test_get_events(self):

Uh oh!

perf(eventstore): get_events fetches event data from nodestore #16230

perf(eventstore): get_events fetches event data from nodestore #16230

Uh oh!

Conversation

lynnagara commented Dec 31, 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!

lynnagara Dec 31, 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!

lynnagara Jan 6, 2020 • 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!

fpacifici left a comment

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!

lynnagara Jan 15, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lynnagara commented Dec 31, 2019 •

edited

Loading

lynnagara Dec 31, 2019 •

edited

Loading

lynnagara Jan 6, 2020 •

edited

Loading

lynnagara Jan 15, 2020 •

edited

Loading