feat:issueless events. Prepare data structures for issueless events #13822

fpacifici · 2019-06-26T06:11:03Z

This PR prepares the ground for skipping issue creation if the event has no fingerprint by removing some assumption that the group should always exists.

src/sentry/similarity/features.py

markstory · 2019-06-26T13:57:27Z

src/sentry/models/event.py

    def group(self):
        from sentry.models import Group
+        if not self.group_id:
+            return None


Do consumers of this property check for nulls?

No, not yet. That is my next step since, as of this PR, there is no code path to create an event without a group so we cannot be in a condition where this will return None yet.

I don't love this but I don't have any better ideas.

As above, I think the only way to avoid this is to split events from transactions/traces. But that opens a different conceptual problem. In a world where the event grouping of choice is the incident, what's the future of groups ? Are they going to still exist ?

mitsuhiko

without running the tests or anything this looks good to me.

fpacifici · 2019-06-26T17:47:35Z

test_featureadoption.py is still failing, fixing it now.

tkaemming · 2019-07-01T18:40:52Z

src/sentry/event_manager.py

                organization_id=project.organization_id,
                project_id=project.id,
-                group_id=group.id,
+                group_id=group.id if group else None,


It's probably worth noting that this will break all of the non-Snuba tagstore implementations (if we haven't already.) We should start putting together a plan to remove the non-Snuba implementations since they'll no longer work correctly.

For issueless events we should not have non-Snuba event at all (even if we are not yet fully there), so by the time we even get to a prototype this should not be a problem.
The plan to remove non Snuba implementation started here :
#13700
in the sense that at least we are querying snuba for events. The further parts of the plan are:

Remove all the explicit Event.objects.whatever (there are 10-20 of them. Mostly in corner cases)

Fix all the tests that create event by writing to postgres instead of using the store_event flow

Address the tagstore implementation

Clean up the code and remove the feature that allows to skip snuba

tkaemming · 2019-07-01T18:51:01Z

src/sentry/similarity/features.py


        items = []
        for event in events:
+            # TODO: how we index events?


Not sure if this is a personal note or what, but probably shouldn't be committed.

In any case, I'm not sure how similarity is going to work with issueless events. For now, my expectation would be that it doesn't work.

Personal note. Should not be committed.
And we should never get to this code with issueless events.

tkaemming · 2019-07-01T18:57:18Z

src/sentry/signals.py

 project_created = BetterSignal(providing_args=["project", "user"])
 first_event_pending = BetterSignal(providing_args=["project", "user"])
-first_event_received = BetterSignal(providing_args=["project", "group"])
+first_event_received = BetterSignal(providing_args=["project", "event", "group"])


Shouldn't group be removed here now that none of the listeners are using/expecting it?

I think you are right, I got to this after fixing a lot of tests, let me try to remove group entirely

tkaemming · 2019-07-01T19:06:05Z

src/sentry/api/serializers/models/event.py

        d = {
            'id': six.text_type(obj.id),
-            'groupID': six.text_type(obj.group_id),
+            'groupID': six.text_type(obj.group_id) if obj.group_id else None,


This change obviously has some pretty wide-ranging implications. Not really sure how to give constructive feedback other than pointing out that hopefully everybody who needs to account for this knows the change is coming.

I'm not too worried other than our UI code breaking. Customers are unlikely to encounter such events as we will not emit webhooks and similar things for issueless events any time soon.

Regarding the breaking change. Overall:
I did an analysis of the impact of groupID and group becoming nullable in several places:
Regarding the backend code, pretty much all the places I found that need the group from the event are unreachable for issueless events. Think about post processing, webhooks, group management code etc. We are not supposed to get there with issueless events.

For what concerns the UI, this is a lot up in the air for now. For what I know so far, the UI over issueless events is different, thank the event details, there are plenty of parts that are conceptually different that would prevent us from reusing the same one even if the group field was not None.

I believe we should eventually have a totally separate data type for issueless events, but this is an open discussion as of now.

The only UI that should bump into events without groupID is the event list page, and it new implementation. The new implementation is expecting issueless events for the most part. However, we're also missing UI components for transactions/spans which will cause different problems (all solvable though).

tkaemming · 2019-07-01T19:23:23Z

src/sentry/models/event.py

    def group(self):
        from sentry.models import Group
+        if not self.group_id:
+            return None


I don't love this but I don't have any better ideas.

This PR prepares the ground for skipping issue creation if the event has no fingerprint by removing some assumption that the group should always exists.

fpacifici requested a review from mitsuhiko June 26, 2019 06:11

fpacifici requested a review from tkaemming as a code owner June 26, 2019 06:11

mitsuhiko suggested changes Jun 26, 2019

View reviewed changes

src/sentry/similarity/features.py Outdated Show resolved Hide resolved

markstory reviewed Jun 26, 2019

View reviewed changes

mitsuhiko approved these changes Jun 26, 2019

View reviewed changes

fpacifici force-pushed the feat/skip_group_creation branch 2 times, most recently from aa79fab to c95aaa4 Compare July 1, 2019 18:18

tkaemming approved these changes Jul 1, 2019

View reviewed changes

fpacifici force-pushed the feat/skip_group_creation branch from 58197cd to 0286c4e Compare July 2, 2019 17:34

fpacifici added 6 commits July 3, 2019 09:22

feat:issueless events. Prepare data structures for issueless events

b9b7dd3

This PR prepares the ground for skipping issue creation if the event has no fingerprint by removing some assumption that the group should always exists.

Use group_id instead of group where possible

505e665

Fix tests

6799fd4

Beef up test coverage

98f90f8

remvoe group where not needed

f2c2a35

Remove dependency from signal on group

0732902

fpacifici force-pushed the feat/skip_group_creation branch from 0286c4e to 0732902 Compare July 3, 2019 16:22

fpacifici merged commit 618bb0c into master Jul 3, 2019

markstory deleted the feat/skip_group_creation branch July 3, 2019 20:29

tkaemming mentioned this pull request Jul 5, 2019

feat(issueless events) Skip group specific phases in post_process for issueless events #13899

Merged

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

Uh oh!

feat:issueless events. Prepare data structures for issueless events #13822

feat:issueless events. Prepare data structures for issueless events #13822

Uh oh!

Conversation

fpacifici commented Jun 26, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fpacifici Jun 26, 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!

mitsuhiko left a comment

Choose a reason for hiding this comment

Uh oh!

fpacifici commented Jun 26, 2019

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fpacifici Jun 26, 2019 •

edited

Loading