Skip to content

ref(feedback): move user report event linking to post_process#70197

Merged
JoshFerge merged 1 commit into
masterfrom
jferg/user-report-post-process
May 3, 2024
Merged

ref(feedback): move user report event linking to post_process#70197
JoshFerge merged 1 commit into
masterfrom
jferg/user-report-post-process

Conversation

@JoshFerge

Copy link
Copy Markdown
Member

currently we link user reports to groups in event_manager. we'd like to move this to post_process to get it out of the main ingestion path.

This PR simply copies the code over from the event manager.

We'll do a follow up PR to delete the code from event manager after this has fully deployed, as to not miss any links.

@JoshFerge JoshFerge requested a review from a team as a code owner May 2, 2024 23:28
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 2, 2024
@JoshFerge JoshFerge requested a review from a team May 2, 2024 23:31
@JoshFerge JoshFerge requested a review from a team May 2, 2024 23:50
)

if user_reports_updated:
metrics.incr("event_manager.save._update_user_reports_with_event_link_updated")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this metric is supposed to show how many were updated, then increment by the row count.

Suggested change
metrics.incr("event_manager.save._update_user_reports_with_event_link_updated")
metrics.incr("event_manager.save._update_user_reports_with_event_link_updated", user_reports_updated)

If it's meant to be just a call count, leave it as is.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be synonymous with call count as the number of times that an event is linked to more than one report is near 0

Comment on lines +1431 to +1434
else:
UserReport.objects.filter(project_id=project.id, event_id=job["event"].event_id).update(
group_id=group.id, environment_id=event.get_environment().id
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If overwriting all is fine, then why not just do that instead of trying to get null ones?
AFAIK an update to same value as already in row should be a no-op in PostgreSQL.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is what we originally did, but am about to make an update where we don't simply want to overwrite all, as we want only the reports that do not currently have a group associated #70200

@JoshFerge JoshFerge merged commit 26eeb0a into master May 3, 2024
@JoshFerge JoshFerge deleted the jferg/user-report-post-process branch May 3, 2024 00:07
JoshFerge added a commit that referenced this pull request May 3, 2024
moved to post_process in jferg/user-report-post-process

See:
* #70197
@sentry

sentry Bot commented May 11, 2024

Copy link
Copy Markdown
Contributor

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Environment.DoesNotExist: Environment matching query does not exist. sentry.tasks.post_process.post_process_group View Issue

Did you find this useful? React with a 👍 or 👎

@github-actions github-actions Bot locked and limited conversation to collaborators May 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants