Skip to content

Commit 9ccc5d2

Browse files
authored
feat(feedback): shim feedback v2 to postgres so they can be queried by group_id (#89685)
1 parent 93911bd commit 9ccc5d2

File tree

10 files changed

+309
-50
lines changed

10 files changed

+309
-50
lines changed

Diff for: src/sentry/feedback/usecases/create_feedback.py

+12-2
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,15 @@ def should_filter_feedback(
283283
return False, None
284284

285285

286-
def create_feedback_issue(event, project_id: int, source: FeedbackCreationSource):
286+
def create_feedback_issue(
287+
event: dict[str, Any], project_id: int, source: FeedbackCreationSource
288+
) -> dict[str, Any] | None:
289+
"""
290+
Produces a feedback issue occurrence to kafka for issues processing. Applies filters, spam filters, and event validation.
291+
292+
Returns the formatted event data that was sent to issue platform.
293+
"""
294+
287295
metrics.incr(
288296
"feedback.create_feedback_issue.entered",
289297
tags={
@@ -306,7 +314,7 @@ def create_feedback_issue(event, project_id: int, source: FeedbackCreationSource
306314
category=DataCategory.USER_REPORT_V2,
307315
quantity=1,
308316
)
309-
return
317+
return None
310318

311319
feedback_message = event["contexts"]["feedback"]["message"]
312320

@@ -417,6 +425,8 @@ def create_feedback_issue(event, project_id: int, source: FeedbackCreationSource
417425
quantity=1,
418426
)
419427

428+
return event_fixed
429+
420430

421431
def auto_ignore_spam_feedbacks(project, issue_fingerprint):
422432
"""

Diff for: src/sentry/feedback/usecases/save_feedback_event.py

+66
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import logging
2+
from collections.abc import Mapping
3+
from datetime import UTC, datetime
4+
from typing import Any
5+
6+
from sentry.feedback.usecases.create_feedback import FeedbackCreationSource, create_feedback_issue
7+
from sentry.ingest.userreport import save_userreport
8+
from sentry.models.project import Project
9+
from sentry.utils import metrics
10+
11+
logger = logging.getLogger(__name__)
12+
13+
14+
def save_feedback_event(event_data: Mapping[str, Any], project_id: int):
15+
"""Saves a feedback from a feedback event envelope.
16+
17+
If the save is successful and the `associated_event_id` field is present, this will
18+
also save a UserReport in Postgres. This is to ensure the feedback can be queried by
19+
group_id, which is hard to associate in clickhouse.
20+
"""
21+
if not isinstance(event_data, dict):
22+
event_data = dict(event_data)
23+
24+
# Produce to issue platform
25+
fixed_event_data = create_feedback_issue(
26+
event_data, project_id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE
27+
)
28+
if not fixed_event_data:
29+
return
30+
31+
try:
32+
# Shim to UserReport
33+
feedback_context = fixed_event_data["contexts"]["feedback"]
34+
associated_event_id = feedback_context.get("associated_event_id")
35+
if associated_event_id:
36+
project = Project.objects.get_from_cache(id=project_id)
37+
save_userreport(
38+
project,
39+
{
40+
"event_id": associated_event_id,
41+
"project_id": project_id,
42+
# XXX(aliu): including environment ensures the update_user_reports task
43+
# will not shim the report back to feedback.
44+
"environment_id": fixed_event_data.get("environment", "production"),
45+
"name": feedback_context["name"],
46+
"email": feedback_context["contact_email"],
47+
"comments": feedback_context["message"],
48+
},
49+
FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE,
50+
start_time=datetime.fromtimestamp(fixed_event_data["timestamp"], UTC),
51+
)
52+
metrics.incr("feedback.shim_to_userreport.success")
53+
54+
except Exception:
55+
metrics.incr("feedback.shim_to_userreport.failed")
56+
logger.exception(
57+
"Error shimming from feedback event to user report.",
58+
extra={
59+
"associated_event_id": associated_event_id,
60+
"project_id": project_id,
61+
"environment_id": fixed_event_data.get("environment"),
62+
"username": feedback_context.get("name"),
63+
"email": feedback_context.get("contact_email"),
64+
"comments": feedback_context.get("message"),
65+
},
66+
)

Diff for: src/sentry/ingest/userreport.py

+8-2
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,10 @@ def save_userreport(
8383
report["event_id"] = report["event_id"].lower()
8484
report["project_id"] = project.id
8585

86-
event = eventstore.backend.get_event_by_id(project.id, report["event_id"])
86+
# Use the associated event to validate and update the report.
87+
event: Event | GroupEvent | None = eventstore.backend.get_event_by_id(
88+
project.id, report["event_id"]
89+
)
8790

8891
euser = find_event_user(event)
8992

@@ -99,6 +102,7 @@ def save_userreport(
99102
report["environment_id"] = event.get_environment().id
100103
report["group_id"] = event.group_id
101104

105+
# Save the report.
102106
try:
103107
with atomic_transaction(using=router.db_for_write(UserReport)):
104108
report_instance = UserReport.objects.create(**report)
@@ -136,6 +140,7 @@ def save_userreport(
136140
if report_instance.group_id:
137141
report_instance.notify()
138142

143+
# Additionally processing if save is successful.
139144
user_feedback_received.send_robust(project=project, sender=save_userreport)
140145

141146
logger.info(
@@ -150,12 +155,13 @@ def save_userreport(
150155
"user_report.create_user_report.saved",
151156
tags={"has_event": bool(event), "referrer": source.value},
152157
)
153-
if event:
158+
if event and source.value in FeedbackCreationSource.old_feedback_category_values():
154159
logger.info(
155160
"ingest.user_report.shim_to_feedback",
156161
extra={"project_id": project.id, "event_id": report["event_id"]},
157162
)
158163
shim_to_feedback(report, event, project, source)
164+
# XXX(aliu): the update_user_reports task will still try to shim the report after a period, unless group_id or environment is set.
159165

160166
return report_instance
161167

Diff for: src/sentry/tasks/post_process.py

+18-17
Original file line numberDiff line numberDiff line change
@@ -1417,25 +1417,26 @@ def link_event_to_user_report(job: PostProcessJob) -> None:
14171417
event = job["event"]
14181418
project = event.project
14191419
user_reports_without_group = UserReport.objects.filter(
1420-
project_id=project.id,
1421-
event_id=event.event_id,
1422-
group_id__isnull=True,
1423-
environment_id__isnull=True,
1420+
project_id=project.id, event_id=event.event_id, group_id__isnull=True
14241421
)
14251422
for report in user_reports_without_group:
1426-
shim_to_feedback(
1427-
{
1428-
"name": report.name,
1429-
"email": report.email,
1430-
"comments": report.comments,
1431-
"event_id": report.event_id,
1432-
"level": "error",
1433-
},
1434-
event,
1435-
project,
1436-
FeedbackCreationSource.USER_REPORT_ENVELOPE,
1437-
)
1438-
metrics.incr("event_manager.save._update_user_reports_with_event_link.shim_to_feedback")
1423+
if report.environment_id is None:
1424+
shim_to_feedback(
1425+
{
1426+
"name": report.name,
1427+
"email": report.email,
1428+
"comments": report.comments,
1429+
"event_id": report.event_id,
1430+
"level": "error",
1431+
},
1432+
event,
1433+
project,
1434+
FeedbackCreationSource.USER_REPORT_ENVELOPE,
1435+
)
1436+
metrics.incr(
1437+
"event_manager.save._update_user_reports_with_event_link.shim_to_feedback"
1438+
)
1439+
# If environment is set, this report was already shimmed from new feedback.
14391440

14401441
user_reports_updated = user_reports_without_group.update(
14411442
group_id=group.id, environment_id=event.get_environment().id

Diff for: src/sentry/tasks/store.py

+2-2
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
from sentry.constants import DEFAULT_STORE_NORMALIZER_ARGS
1717
from sentry.datascrubbing import scrub_data
1818
from sentry.eventstore import processing
19-
from sentry.feedback.usecases.create_feedback import FeedbackCreationSource, create_feedback_issue
19+
from sentry.feedback.usecases.save_feedback_event import save_feedback_event
2020
from sentry.ingest.types import ConsumerType
2121
from sentry.killswitches import killswitch_matches_context
2222
from sentry.lang.native.symbolicator import SymbolicatorTaskKind
@@ -690,7 +690,7 @@ def save_event_feedback(
690690
project_id: int,
691691
**kwargs: Any,
692692
) -> None:
693-
create_feedback_issue(data, project_id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE)
693+
save_feedback_event(data, project_id)
694694

695695

696696
@instrumented_task(

Diff for: src/sentry/tasks/update_user_reports.py

+21-20
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,12 @@ def update_user_reports(**kwargs: Any) -> None:
3838
# ingestion time
3939
user_reports = UserReport.objects.filter(
4040
group_id__isnull=True,
41-
environment_id__isnull=True,
4241
date_added__gte=start,
4342
date_added__lte=end,
4443
)
4544

4645
# We do one query per project, just to avoid the small case that two projects have the same event ID
47-
project_map: dict[int, Any] = {}
46+
project_map: dict[int, list[UserReport]] = {}
4847
for r in user_reports:
4948
project_map.setdefault(r.project_id, []).append(r)
5049

@@ -90,24 +89,26 @@ def update_user_reports(**kwargs: Any) -> None:
9089
for event in events:
9190
report = report_by_event.get(event.event_id)
9291
if report:
93-
if not is_in_feedback_denylist(project.organization):
94-
logger.info(
95-
"update_user_reports.shim_to_feedback",
96-
extra={"report_id": report.id, "event_id": event.event_id},
97-
)
98-
metrics.incr("tasks.update_user_reports.shim_to_feedback")
99-
shim_to_feedback(
100-
{
101-
"name": report.name,
102-
"email": report.email,
103-
"comments": report.comments,
104-
"event_id": report.event_id,
105-
"level": "error",
106-
},
107-
event,
108-
project,
109-
FeedbackCreationSource.UPDATE_USER_REPORTS_TASK,
110-
)
92+
if report.environment_id is None:
93+
if not is_in_feedback_denylist(project.organization):
94+
logger.info(
95+
"update_user_reports.shim_to_feedback",
96+
extra={"report_id": report.id, "event_id": event.event_id},
97+
)
98+
metrics.incr("tasks.update_user_reports.shim_to_feedback")
99+
shim_to_feedback(
100+
{
101+
"name": report.name,
102+
"email": report.email,
103+
"comments": report.comments,
104+
"event_id": report.event_id,
105+
"level": "error",
106+
},
107+
event,
108+
project,
109+
FeedbackCreationSource.UPDATE_USER_REPORTS_TASK,
110+
)
111+
# XXX(aliu): If a report has environment_id but not group_id, this report was shimmed from a feedback issue, so no need to shim again.
111112
report.update(group_id=event.group_id, environment_id=event.get_environment().id)
112113
updated_reports += 1
113114

Diff for: tests/sentry/feedback/usecases/test_create_feedback.py

+9-6
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,10 @@ def create_dummy_response(*args, **kwargs):
7474
)
7575

7676

77-
def mock_feedback_event(project_id: int, dt: datetime):
77+
def mock_feedback_event(project_id: int, dt: datetime | None = None):
78+
if dt is None:
79+
dt = datetime.now(UTC)
80+
7881
return {
7982
"project_id": project_id,
8083
"request": {
@@ -773,7 +776,7 @@ def test_create_feedback_adds_associated_event_id(
773776
@django_db_all
774777
def test_create_feedback_tags(default_project, mock_produce_occurrence_to_kafka):
775778
"""We want to surface these tags in the UI. We also use user.email for alert conditions."""
776-
event = mock_feedback_event(default_project.id, datetime.now(UTC))
779+
event = mock_feedback_event(default_project.id)
777780
event["user"]["email"] = "[email protected]"
778781
event["contexts"]["feedback"]["contact_email"] = "[email protected]"
779782
event["contexts"]["trace"] = {"trace_id": "abc123"}
@@ -818,7 +821,7 @@ def test_create_feedback_tags_no_associated_event_id(
818821

819822
@django_db_all
820823
def test_create_feedback_tags_skips_if_empty(default_project, mock_produce_occurrence_to_kafka):
821-
event = mock_feedback_event(default_project.id, datetime.now(UTC))
824+
event = mock_feedback_event(default_project.id)
822825
event["user"].pop("email", None)
823826
event["contexts"]["feedback"].pop("contact_email", None)
824827
create_feedback_issue(event, default_project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE)
@@ -848,7 +851,7 @@ def test_create_feedback_filters_large_message(
848851
monkeypatch.setattr("sentry.llm.usecases.complete_prompt", mock_complete_prompt)
849852

850853
with Feature(features), set_sentry_option("feedback.message.max-size", 4096):
851-
event = mock_feedback_event(default_project.id, datetime.now(UTC))
854+
event = mock_feedback_event(default_project.id)
852855
event["contexts"]["feedback"]["message"] = "a" * 7007
853856
create_feedback_issue(
854857
event, default_project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE
@@ -861,7 +864,7 @@ def test_create_feedback_filters_large_message(
861864
@django_db_all
862865
def test_create_feedback_evidence_has_source(default_project, mock_produce_occurrence_to_kafka):
863866
"""We need this evidence field in post process, to determine if we should send alerts."""
864-
event = mock_feedback_event(default_project.id, datetime.now(UTC))
867+
event = mock_feedback_event(default_project.id)
865868
source = FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE
866869
create_feedback_issue(event, default_project.id, source)
867870

@@ -879,7 +882,7 @@ def test_create_feedback_evidence_has_spam(
879882
default_project.update_option("sentry:feedback_ai_spam_detection", True)
880883

881884
with Feature({"organizations:user-feedback-spam-filter-ingest": True}):
882-
event = mock_feedback_event(default_project.id, datetime.now(UTC))
885+
event = mock_feedback_event(default_project.id)
883886
source = FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE
884887
create_feedback_issue(event, default_project.id, source)
885888

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
from datetime import UTC, datetime
2+
from unittest import mock
3+
4+
import pytest
5+
6+
from sentry.feedback.usecases.create_feedback import FeedbackCreationSource
7+
from sentry.feedback.usecases.save_feedback_event import save_feedback_event
8+
from sentry.testutils.pytest.fixtures import django_db_all
9+
from tests.sentry.feedback.usecases.test_create_feedback import mock_feedback_event
10+
11+
12+
@pytest.fixture
13+
def mock_create_feedback_issue():
14+
with mock.patch("sentry.feedback.usecases.save_feedback_event.create_feedback_issue") as m:
15+
yield m
16+
17+
18+
@pytest.fixture
19+
def mock_save_userreport():
20+
with mock.patch("sentry.feedback.usecases.save_feedback_event.save_userreport") as m:
21+
yield m
22+
23+
24+
@django_db_all
25+
def test_save_feedback_event_no_associated_error(
26+
default_project, mock_create_feedback_issue, mock_save_userreport
27+
):
28+
event_data = mock_feedback_event(default_project.id)
29+
mock_create_feedback_issue.return_value = None
30+
31+
save_feedback_event(event_data, default_project.id)
32+
33+
mock_create_feedback_issue.assert_called_once_with(
34+
event_data, default_project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE
35+
)
36+
mock_save_userreport.assert_not_called()
37+
38+
39+
@django_db_all
40+
def test_save_feedback_event_with_associated_error(
41+
default_project, mock_create_feedback_issue, mock_save_userreport
42+
):
43+
event_data = mock_feedback_event(default_project.id)
44+
event_data["contexts"]["feedback"]["associated_event_id"] = "abcd" * 8
45+
mock_create_feedback_issue.return_value = event_data
46+
47+
save_feedback_event(event_data, default_project.id)
48+
49+
mock_create_feedback_issue.assert_called_once_with(
50+
event_data, default_project.id, FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE
51+
)
52+
53+
feedback_context = event_data["contexts"]["feedback"]
54+
mock_save_userreport.assert_called_once_with(
55+
default_project,
56+
{
57+
"event_id": "abcd" * 8,
58+
"project_id": default_project.id,
59+
"environment_id": event_data["environment"],
60+
"name": feedback_context["name"],
61+
"email": feedback_context["contact_email"],
62+
"comments": feedback_context["message"],
63+
},
64+
FeedbackCreationSource.NEW_FEEDBACK_ENVELOPE,
65+
start_time=datetime.fromtimestamp(event_data["timestamp"], UTC),
66+
)

0 commit comments

Comments
 (0)