feat: Event / SnubaEvent compatibility #16222
Conversation
Make eventstore Event model compatible with SnubaEvent, which will allow us to deprecate SnubaEvent everywhere and move towards a single Event model.
lynnagara
force-pushed
the
snubaevent-compatibility
branch
2 times, most recently
from
f758aeb to
cade6f9
Compare
lynnagara
force-pushed
the
snubaevent-compatibility
branch
from
c3984fc to
c76b2b0
Compare
src/sentry/eventstore/models.py
Outdated
| if column in self._snuba_data and self._snuba_data[column]: | ||
| return self._snuba_data[column] | ||
| return None |
There was a problem hiding this comment.
return self._snuba_data.get(column)
?
| self.group_id = group_id | ||
| self.message = message | ||
| self.data = data | ||
| self._snuba_data = snuba_data or {} |
There was a problem hiding this comment.
How long do you expect this to be around ?
Asking because I would like to avoid the concern that the two implementations (this one and the snuba one could diverge)
There was a problem hiding this comment.
Are you referring to this line or the SnubaEvent class in general? This line will probably stay to hold data fetched from Snuba. SnubaEvent will go away soon, outisde this PR it's only used in a couple of small cases.
There was a problem hiding this comment.
The snuba event class that has a copy paste of a lot of the logic you added here to the Event class.
There was a problem hiding this comment.
We'll be able to remove SnubaEvent really soon
| self.project_id = project_id | ||
| self.event_id = event_id | ||
| self.group_id = group_id | ||
| self.message = message |
There was a problem hiding this comment.
why was this promoted to a field of the Event class ?
There was a problem hiding this comment.
The EventManager sets the event.message property to be something else (the full search message) than what event.message is the rest of the time :/ The Django event also had a top level message field that held this value as well. It might be better to give this a distinct name like search_message or something.
| @property | ||
| def timestamp(self): | ||
| column = self.__get_column_name(Columns.TIMESTAMP) | ||
| if column in self._snuba_data: | ||
| return self._snuba_data[column] | ||
| return self.datetime.isoformat() |
There was a problem hiding this comment.
IS this here just because SnubaEvent has one ?
There was a problem hiding this comment.
Sorry, which part are you referring to?
There was a problem hiding this comment.
The existence of both datetime and timestamp
There was a problem hiding this comment.
Yes, we rely on event.timestamp in various places
fpacifici
left a comment
fpacifici
left a comment
There was a problem hiding this comment.
Seems there are some test failure around setting the message field on event that are actual issues.
This reverts commit 7d68e30.
Make eventstore Event model compatible with SnubaEvent, which will allow us to deprecate SnubaEvent everywhere and move towards a single Event model.
Make eventstore Event model compatible with SnubaEvent, which will allow
us to deprecate SnubaEvent everywhere and move towards a single Event
model.