feat(eventuser): Migrate IssuesByTagProcessor away from EventUser #59672

NisanthanNanthakumar · 2023-11-09T01:31:30Z

Objective:

Create the for_tags method in the EventUser dataclass. Migrates get_eventuser_callback to use the dataclass.

wedamija · 2023-11-09T19:01:45Z

src/sentry/data_export/processors/issues_by_tag.py

@@ -97,7 +97,7 @@ def serialize_row(item, key):
        }
        if key == "user":
            euser = item._eventuser
-            result["id"] = euser.ident if euser else ""
+            result["id"] = euser.user_id if euser else ""


Why do we stop using ident here?

Hmm nm, I remember your comment here:

@wedamija the dataclass's id is to mirror the EventUser model id column. user_id in the dataclass will replace the ident column in EventUser

Although now that I'm seeing it it is a little confusing to call it user_id since it implies an id from the User model. Should this at least be user_ident or something?

The EventUser dataclass does not have the property ident. ident in the EventUser model is equivalent to the user_id. This is where we create the EventUser row and you can see that ident is from user.id

yep! I am ok to rename to user_ident

Updated here in this PR #59590

And updated this PR

wedamija · 2023-11-09T21:12:37Z

src/sentry/utils/eventuser.py

+        result = {}
+        for value in values:
+            keyword_filters = {value.split(":", 1)[0]: value.split(":", 1)[-1]}
+            eventusers = EventUser.for_projects(projects, keyword_filters)


Nit: Tbh we should probably change for_projects to just return an EventUser dataclass or None, rather than having to unwrap it like this.

src/sentry/utils/eventuser.py

codecov · 2023-11-13T19:42:43Z

Codecov Report

Merging #59672 (4a8ac6d) into master (2927744) will increase coverage by 0.14%.
Report is 135 commits behind head on master.
The diff coverage is 93.54%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #59672      +/-   ##
==========================================
+ Coverage   80.61%   80.76%   +0.14%     
==========================================
  Files        5175     5180       +5     
  Lines      227259   227578     +319     
  Branches    38243    38289      +46     
==========================================
+ Hits       183214   183795     +581     
+ Misses      38464    38187     -277     
- Partials     5581     5596      +15

Files	Coverage Δ
src/sentry/analytics/events/__init__.py	`100.00% <100.00%> (ø)`
...c/sentry/analytics/events/eventuser_snuba_query.py	`100.00% <100.00%> (ø)`
src/sentry/data_export/processors/issues_by_tag.py	`100.00% <100.00%> (ø)`
src/sentry/search/utils.py	`84.19% <100.00%> (ø)`
src/sentry/utils/eventuser.py	`94.28% <92.30%> (+16.65%)`	⬆️

... and 129 files with indirect coverage changes

wedamija · 2023-11-13T20:50:46Z

src/sentry/utils/eventuser.py

        )

+        if return_all:


if not return_all?

src/sentry/utils/eventuser.py

wedamija · 2023-11-14T17:52:58Z

src/sentry/utils/eventuser.py

+    def _find_matching_items(
+        snuba_results: List[dict[str, Any]], filters: Mapping[str, Any], filter_boolean: str
+    ):


We don't need to match on the keyword filters. The way to find a unique EventUser is to look at the sentry:user tag, which maps to the user column in Snuba. The value is assigned here:

sentry/src/sentry/event_manager.py

Line 1064 in 27e7250

set_tag(data, "sentry:user", user.tag_value)

So the user column will just be the result of

sentry/src/sentry/models/eventuser.py

Lines 85 to 91 in fda93e5

def tag_value(self):

"""

Return the identifier used with tags to link this user.

"""

for key, value in self.iter_attributes():

if value:

return f"{KEYWORD_MAP[key]}:{value}"

That tag is mapped to a column here: https://github.com/getsentry/snuba/blob/94c47010b1c6425942684b54d3ead222c0ad181a/snuba/datasets/configuration/events/storages/errors.yaml#L350

So if we had a window function, we'd partition on user and fetch the first row for each user. In this case, we have to group on all the cols and just select the most recent user in code instead.

oh lol! that would have saved a lot of time yesterday 😭. This simplifies the logic alot.

wedamija · 2023-11-14T17:56:59Z

src/sentry/utils/eventuser.py

+            snuba_keyword_filters[snuba_column] = value
+
+        matches = self._find_matching_items(data_results, snuba_keyword_filters, filter_boolean)
+        first_matching_items.extend(matches.values())


Should we just assign directly here? Or just use matches.values() directly to get results?

This logic has been removed with the simplification of just using tag_value to get the most recent unique EventUsers

wedamija · 2023-11-14T17:57:47Z

src/sentry/utils/eventuser.py

+
+        columns = [
+            Column("project_id"),
+            Column("group_id"),


What is group_id for here?

Ah I removed it. Its a legacy column from the original snuba query for analytics.
I also removed the user column from the Snuba query.

wedamija

lgtm! Tests are pretty thorough, thanks.

Make sure you monitor this deploy and be ready for a rollback in case we see anything going wrong. Ideally just at a time when we're both not in meetings.

wedamija · 2023-11-15T00:54:17Z

src/sentry/utils/eventuser.py

+        self,
+        projects: List[Project],
+        keyword_filters: Mapping[str, List[Any]],
+        filter_boolean="AND",


Nit: Wondering if this should just be the BooleanOp

wedamija · 2023-11-15T00:55:30Z

src/sentry/utils/eventuser.py

+            project_ids=[p.id for p in projects],
+            query=query.print(),
+            count_rows_returned=len(data_results),
+            count_rows_filtered=len(data_results) - len(results),


Could also be worth including query time here

does raw_snql_query func return this?

Hmm, might be in the response, I don't remember. Was thinking you could just keep your own timer here

wedamija · 2023-11-15T01:12:31Z

src/sentry/utils/eventuser.py

@@ -72,7 +73,10 @@ def from_event(event: Event) -> EventUser:
            project_id=event.project_id if event else None,
            email=event.data.get("user", {}).get("email") if event else None,
            username=event.data.get("user", {}).get("username") if event else None,
-            name=event.data.get("user", {}).get("name") if event else None,
+            name=event.data.get("user", {}).get("name")
+            or event.data.get("user", {}).get("username")


Hmm, I'm not totally sure we should put username here, should be ok to leave it blank if not provided (unless there's an example elsewhere where we substitute in username)

sounds good!

wedamija · 2023-11-15T01:24:23Z

src/sentry/utils/eventuser.py

        for keyword, value in keyword_filters.items():
+            if not isinstance(value, list):
+                raise Exception(f"{keyword} filter must be a list of values")


Nit: ValueError?

getsentry-bot · 2023-11-16T01:04:57Z

PR reverted: a84ed80

…User (#59672)" This reverts commit e3f4723. Co-authored-by: NisanthanNanthakumar <[email protected]>

…ntUser (#60071) ## Objective: Create the for_tags method in the EventUser dataclass. Migrates get_eventuser_callback to use the dataclass. Unreverts #59672

NisanthanNanthakumar requested a review from a team November 9, 2023 01:31

NisanthanNanthakumar marked this pull request as ready for review November 9, 2023 01:31

NisanthanNanthakumar requested a review from a team November 9, 2023 01:31

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 9, 2023

vercel bot deployed to Preview November 9, 2023 01:33 View deployment

NisanthanNanthakumar force-pushed the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch from 08ca9e4 to eba2bdb Compare November 9, 2023 01:47

vercel bot deployed to Preview November 9, 2023 01:50 View deployment

NisanthanNanthakumar force-pushed the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch from eba2bdb to 9eca754 Compare November 9, 2023 18:54

vercel bot deployed to Preview November 9, 2023 18:56 View deployment

wedamija reviewed Nov 9, 2023

View reviewed changes

NisanthanNanthakumar force-pushed the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch from 9eca754 to 6bba8b7 Compare November 9, 2023 20:35

NisanthanNanthakumar requested a review from wedamija November 9, 2023 20:38

vercel bot deployed to Preview November 9, 2023 20:40 View deployment

wedamija reviewed Nov 9, 2023

View reviewed changes

src/sentry/utils/eventuser.py Show resolved Hide resolved

src/sentry/utils/eventuser.py Show resolved Hide resolved

vercel bot deployed to Preview November 9, 2023 22:06 View deployment

NisanthanNanthakumar requested a review from wedamija November 9, 2023 22:09

NisanthanNanthakumar force-pushed the 59399-migrate-search-util-function-away-from-eventuser-model branch from 18c8ee0 to c2a8110 Compare November 9, 2023 22:21

vercel bot deployed to Preview November 9, 2023 22:24 View deployment

NisanthanNanthakumar force-pushed the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch from 19fdff2 to e4a6c1b Compare November 9, 2023 22:52

vercel bot deployed to Preview November 9, 2023 22:55 View deployment

NisanthanNanthakumar force-pushed the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch from e4a6c1b to 2179614 Compare November 10, 2023 01:27

vercel bot deployed to Preview November 10, 2023 01:30 View deployment

vercel bot deployed to Preview November 10, 2023 02:16 View deployment

NisanthanNanthakumar force-pushed the 59399-migrate-search-util-function-away-from-eventuser-model branch from 48747c3 to 02571b5 Compare November 13, 2023 18:53

NisanthanNanthakumar force-pushed the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch from f37acc4 to e59edf3 Compare November 13, 2023 18:55

vercel bot deployed to Preview November 13, 2023 18:58 View deployment

vercel bot deployed to Preview November 13, 2023 19:08 View deployment

vercel bot deployed to Preview November 13, 2023 19:54 View deployment

wedamija reviewed Nov 13, 2023

View reviewed changes

NisanthanNanthakumar added 2 commits November 13, 2023 18:39

groupby and filter

b822723

add analytics

bd87f65

NisanthanNanthakumar requested a review from wedamija November 14, 2023 03:14

vercel bot deployed to Preview November 14, 2023 03:16 View deployment

fix analytic event attribute

f49869c

vercel bot deployed to Preview November 14, 2023 04:57 View deployment

fixes

1173c6d

vercel bot deployed to Preview November 14, 2023 05:24 View deployment

wedamija reviewed Nov 14, 2023

View reviewed changes

NisanthanNanthakumar added 2 commits November 14, 2023 14:37

rm complicated logic

2a2f0ac

remove user column

71cf485

NisanthanNanthakumar requested a review from wedamija November 14, 2023 22:41

vercel bot deployed to Preview November 14, 2023 22:42 View deployment

wedamija approved these changes Nov 15, 2023

View reviewed changes

fixes

b0fcd4b

vercel bot deployed to Preview November 15, 2023 21:48 View deployment

undo comment

4a8ac6d

vercel bot deployed to Preview November 15, 2023 22:25 View deployment

NisanthanNanthakumar merged commit e3f4723 into master Nov 15, 2023
49 checks passed

NisanthanNanthakumar deleted the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch November 15, 2023 23:02

NisanthanNanthakumar added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Nov 16, 2023

getsentry-bot added a commit that referenced this pull request Nov 16, 2023

Revert "feat(eventuser): Migrate IssuesByTagProcessor away from Event…

a84ed80

…User (#59672)" This reverts commit e3f4723. Co-authored-by: NisanthanNanthakumar <[email protected]>

NisanthanNanthakumar restored the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch November 16, 2023 03:22

NisanthanNanthakumar mentioned this pull request Nov 16, 2023

unrevert(feat(eventuser)): Migrate IssuesByTagProcessor away from EventUser #60071

Merged

wedamija deleted the 59394-migrate-issuesbytagprocessor-away-from-eventuser-model branch November 16, 2023 18:12

github-actions bot locked and limited conversation to collaborators Dec 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eventuser): Migrate IssuesByTagProcessor away from EventUser #59672

feat(eventuser): Migrate IssuesByTagProcessor away from EventUser #59672

NisanthanNanthakumar commented Nov 9, 2023

wedamija Nov 9, 2023

wedamija Nov 9, 2023

NisanthanNanthakumar Nov 9, 2023

NisanthanNanthakumar Nov 9, 2023

NisanthanNanthakumar Nov 9, 2023

NisanthanNanthakumar Nov 9, 2023

wedamija Nov 9, 2023

codecov bot commented Nov 13, 2023 •

edited

Loading

wedamija Nov 13, 2023

wedamija Nov 14, 2023

NisanthanNanthakumar Nov 14, 2023

wedamija Nov 14, 2023

NisanthanNanthakumar Nov 14, 2023

wedamija Nov 14, 2023

NisanthanNanthakumar Nov 14, 2023

wedamija left a comment

wedamija Nov 15, 2023

wedamija Nov 15, 2023

NisanthanNanthakumar Nov 15, 2023

wedamija Nov 15, 2023

wedamija Nov 15, 2023

NisanthanNanthakumar Nov 15, 2023

wedamija Nov 15, 2023

getsentry-bot commented Nov 16, 2023

	def tag_value(self):
	"""
	Return the identifier used with tags to link this user.
	"""
	for key, value in self.iter_attributes():
	if value:
	return f"{KEYWORD_MAP[key]}:{value}"

feat(eventuser): Migrate IssuesByTagProcessor away from EventUser #59672

feat(eventuser): Migrate IssuesByTagProcessor away from EventUser #59672

Conversation

NisanthanNanthakumar commented Nov 9, 2023

Objective:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wedamija left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getsentry-bot commented Nov 16, 2023

codecov bot commented Nov 13, 2023 •

edited

Loading