Skip to content

Commit

Permalink
fix(grouping): Add normalized SDK name as a tag on event_manager + fix (
Browse files Browse the repository at this point in the history 
#59736)

#59572 + fix for `TypeError`

- validated that [new
test](bb4771d)
causes `TypeError` without the fix
- validated that the fix works even without `try/except`
- added generic try/except as defense in depth 


[Diff for just the fix +
test](https://github.com/getsentry/sentry/pull/59736/files/38f868af62fb13a88993e83a2048852309c0964c..bb4771d6f1330375080d4d4526599d780b0293ae)

---------

Co-authored-by: Katie Byers <[email protected]>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
  • Loading branch information
@lobsterkatie @getsantry
3 people authored Nov 9, 2023
1 parent b0bb1bf commit 27e7250
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 44 deletions.
123 changes: 82 additions & 41 deletions src/sentry/event_manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@
from sentry.utils.performance_issues.performance_detection import detect_performance_problems
from sentry.utils.performance_issues.performance_problem import PerformanceProblem
from sentry.utils.safe import get_path, safe_execute, setdefault_path, trim
from sentry.utils.tag_normalization import normalize_sdk_tag

if TYPE_CHECKING:
from sentry.eventstore.models import BaseEvent, Event
Expand Down Expand Up @@ -178,6 +179,26 @@ def is_sample_event(job):
return get_tag(job["data"], "sample_event") == "yes"


def normalized_sdk_tag_from_event(event: Event) -> str:
"""
Normalize tags coming from SDKs to more manageable canonical form, by:
- combining synonymous tags (`sentry.react` -> `sentry.javascript.react`),
- ignoring framework differences (`sentry.python.flask` and `sentry.python.django` -> `sentry.python`)
- collapsing all community/third-party SDKs into a single `other` category
Note: Some platforms may keep their framework-specific values, as needed for analytics.
This is done to reduce the cardinality of the `sdk.name` tag, while keeping
the ones interesinting to us as granual as possible.
"""
try:
return normalize_sdk_tag((event.data.get("sdk") or {}).get("name") or "other")
except Exception:
logger.warning("failed to get SDK name", exc_info=True)
return "other"


def plugin_is_regression(group: Group, event: Event) -> bool:
project = event.project
for plugin in plugins.for_project(project):
Expand Down Expand Up @@ -451,7 +472,10 @@ def save(

return jobs[0]["event"]
else:
metric_tags = {"platform": job["event"].platform or "unknown"}
metric_tags = {
"platform": job["event"].platform or "unknown",
"sdk": normalized_sdk_tag_from_event(job["event"]),
}
# This metric allows differentiating from all calls to the `event_manager.save` metric
# and adds support for differentiating based on platforms
with metrics.timer("event_manager.save_error_events", tags=metric_tags):
Expand Down Expand Up @@ -776,7 +800,7 @@ def _auto_update_grouping(project: Project) -> None:
"sentry:secondary_grouping_expiry": expiry,
"sentry:grouping_config": new_grouping,
}
for (key, value) in changes.items():
for key, value in changes.items():
project.update_option(key, value)
create_system_audit_entry(
organization=project.organization,
Expand All @@ -786,11 +810,17 @@ def _auto_update_grouping(project: Project) -> None:
)


@metrics.wraps("event_manager.background_grouping")
# TODO: this seems to be dead code, validate and remove
def _calculate_background_grouping(
project: Project, event: Event, config: GroupingConfig
) -> CalculatedHashes:
return _calculate_event_grouping(project, event, config)
metric_tags: MutableTags = {
"grouping_config": config["id"],
"platform": event.platform or "unknown",
"sdk": normalized_sdk_tag_from_event(event),
}
with metrics.timer("event_manager.background_grouping", tags=metric_tags):
return _calculate_event_grouping(project, event, config)


def _run_background_grouping(project: Project, job: Job) -> None:
Expand Down Expand Up @@ -1581,7 +1611,6 @@ def _save_aggregate(
kwargs["data"]["last_received"] = received_timestamp

if existing_grouphash is None:

if killswitch_matches_context(
"store.load-shed-group-creation-projects",
{
Expand Down Expand Up @@ -1621,7 +1650,6 @@ def _save_aggregate(
root_hierarchical_grouphash = None

if existing_grouphash is None:

group = _create_group(project, event, **kwargs)

if (
Expand Down Expand Up @@ -1654,7 +1682,10 @@ def _save_aggregate(
metrics.incr(
"group.created",
skip_internal=True,
tags={"platform": event.platform or "unknown"},
tags={
"platform": event.platform or "unknown",
"sdk": normalized_sdk_tag_from_event(event),
},
)

# This only applies to events with stacktraces
Expand All @@ -1665,6 +1696,7 @@ def _save_aggregate(
sample_rate=1.0,
tags={
"platform": event.platform or "unknown",
"sdk": normalized_sdk_tag_from_event(event),
"frame_mix": frame_mix,
},
)
Expand Down Expand Up @@ -1826,7 +1858,9 @@ def _create_group(project: Project, event: Event, **kwargs: Any) -> Group:
except OperationalError:
metrics.incr(
"next_short_id.timeout",
tags={"platform": event.platform or "unknown"},
tags={
"platform": event.platform or "unknown"
}, # TODO: remove this tag, it's nor relevant
)
sentry_sdk.capture_message("short_id.timeout")
raise HashDiscarded("Timeout when getting next_short_id", reason="timeout")
Expand Down Expand Up @@ -2207,7 +2241,10 @@ def discard_event(job: Job, attachments: Sequence[Attachment]) -> None:
metrics.incr(
"events.discarded",
skip_internal=True,
tags={"platform": job["platform"]},
tags={
"platform": job["platform"],
"sdk": normalized_sdk_tag_from_event(job["event"]),
},
)


Expand Down Expand Up @@ -2455,7 +2492,6 @@ def _materialize_event_metrics(jobs: Sequence[Job]) -> None:
job["event_metrics"] = event_metrics


@metrics.wraps("save_event.calculate_event_grouping")
def _calculate_event_grouping(
project: Project, event: Event, grouping_config: GroupingConfig
) -> CalculatedHashes:
Expand All @@ -2466,40 +2502,42 @@ def _calculate_event_grouping(
metric_tags: MutableTags = {
"grouping_config": grouping_config["id"],
"platform": event.platform or "unknown",
"sdk": normalized_sdk_tag_from_event(event),
}

with metrics.timer("event_manager.normalize_stacktraces_for_grouping", tags=metric_tags):
with sentry_sdk.start_span(op="event_manager.normalize_stacktraces_for_grouping"):
event.normalize_stacktraces_for_grouping(load_grouping_config(grouping_config))

# Detect & set synthetic marker if necessary
detect_synthetic_exception(event.data, grouping_config)

with metrics.timer("event_manager.apply_server_fingerprinting"):
# The active grouping config was put into the event in the
# normalize step before. We now also make sure that the
# fingerprint was set to `'{{ default }}' just in case someone
# removed it from the payload. The call to get_hashes will then
# look at `grouping_config` to pick the right parameters.
event.data["fingerprint"] = event.data.data.get("fingerprint") or ["{{ default }}"]
apply_server_fingerprinting(
event.data.data,
get_fingerprinting_config_for_project(project),
allow_custom_title=True,
)
with metrics.timer("save_event.calculate_event_grouping", tags=metric_tags):
with metrics.timer("event_manager.normalize_stacktraces_for_grouping", tags=metric_tags):
with sentry_sdk.start_span(op="event_manager.normalize_stacktraces_for_grouping"):
event.normalize_stacktraces_for_grouping(load_grouping_config(grouping_config))

# Detect & set synthetic marker if necessary
detect_synthetic_exception(event.data, grouping_config)

with metrics.timer("event_manager.apply_server_fingerprinting", tags=metric_tags):
# The active grouping config was put into the event in the
# normalize step before. We now also make sure that the
# fingerprint was set to `'{{ default }}' just in case someone
# removed it from the payload. The call to get_hashes will then
# look at `grouping_config` to pick the right parameters.
event.data["fingerprint"] = event.data.data.get("fingerprint") or ["{{ default }}"]
apply_server_fingerprinting(
event.data.data,
get_fingerprinting_config_for_project(project),
allow_custom_title=True,
)

with metrics.timer("event_manager.event.get_hashes", tags=metric_tags):
# Here we try to use the grouping config that was requested in the
# event. If that config has since been deleted (because it was an
# experimental grouping config) we fall back to the default.
try:
hashes = event.get_hashes(grouping_config)
except GroupingConfigNotFound:
event.data["grouping_config"] = get_grouping_config_dict_for_project(project)
hashes = event.get_hashes()
with metrics.timer("event_manager.event.get_hashes", tags=metric_tags):
# Here we try to use the grouping config that was requested in the
# event. If that config has since been deleted (because it was an
# experimental grouping config) we fall back to the default.
try:
hashes = event.get_hashes(grouping_config)
except GroupingConfigNotFound:
event.data["grouping_config"] = get_grouping_config_dict_for_project(project)
hashes = event.get_hashes()

hashes.write_to_event(event.data)
return hashes
hashes.write_to_event(event.data)
return hashes


@metrics.wraps("save_event.calculate_span_grouping")
Expand All @@ -2518,7 +2556,10 @@ def _calculate_span_grouping(jobs: Sequence[Job], projects: ProjectsMapping) ->
metrics.incr(
"save_event.transaction.span_group_count.default",
amount=len(unique_default_hashes),
tags={"platform": job["platform"] or "unknown"},
tags={
"platform": job["platform"] or "unknown",
"sdk": normalized_sdk_tag_from_event(event),
},
)
except Exception:
sentry_sdk.capture_exception()
Expand Down
45 changes: 42 additions & 3 deletions tests/sentry/event_manager/test_event_manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2301,7 +2301,6 @@ def test_perf_issue_creation(self):
@override_options({"performance.issues.all.problem-detection": 1.0})
@override_options({"performance.issues.n_plus_one_db.problem-creation": 1.0})
def test_perf_issue_update(self):

with mock.patch("sentry_sdk.tracing.Span.containing_transaction"):
event = self.create_performance_issue(
event_data=make_event(**get_event("n-plus-one-in-django-index-view"))
Expand Down Expand Up @@ -2441,7 +2440,9 @@ def attempt_to_generate_slow_db_issue() -> Event:

@patch("sentry.event_manager.metrics.incr")
def test_new_group_metrics_logging(self, mock_metrics_incr: MagicMock) -> None:
manager = EventManager(make_event(platform="javascript"))
manager = EventManager(
make_event(platform="javascript", sdk={"name": "sentry.javascript.nextjs"})
)
manager.normalize()
manager.save(self.project.id)

Expand All @@ -2450,12 +2451,49 @@ def test_new_group_metrics_logging(self, mock_metrics_incr: MagicMock) -> None:
skip_internal=True,
tags={
"platform": "javascript",
"sdk": "sentry.javascript.nextjs",
},
)

@patch("sentry.event_manager.metrics.incr")
def test_new_group_metrics_logging_no_platform_no_sdk(
self, mock_metrics_incr: MagicMock
) -> None:
manager = EventManager(make_event(platform=None, sdk=None))
manager.normalize()
manager.save(self.project.id)

mock_metrics_incr.assert_any_call(
"group.created",
skip_internal=True,
tags={
"platform": "other",
"sdk": "other",
},
)

@patch("sentry.event_manager.metrics.incr")
def test_new_group_metrics_logging_sdk_exist_but_null(
self, mock_metrics_incr: MagicMock
) -> None:
manager = EventManager(make_event(platform=None, sdk={"name": None}))
manager.normalize()
manager.save(self.project.id)

mock_metrics_incr.assert_any_call(
"group.created",
skip_internal=True,
tags={
"platform": "other",
"sdk": "other",
},
)

def test_new_group_metrics_logging_with_frame_mix(self) -> None:
with patch("sentry.event_manager.metrics.incr") as mock_metrics_incr:
manager = EventManager(make_event(platform="javascript"))
manager = EventManager(
make_event(platform="javascript", sdk={"name": "sentry.javascript.nextjs"})
)
manager.normalize()
# IRL, `normalize_stacktraces_for_grouping` adds frame mix metadata to the event, but we
# can't mock that because it's imported inside its calling function to avoid circular imports
Expand All @@ -2468,6 +2506,7 @@ def test_new_group_metrics_logging_with_frame_mix(self) -> None:
tags={
"platform": "javascript",
"frame_mix": "in-app-only",
"sdk": "sentry.javascript.nextjs",
},
)

Expand Down

0 comments on commit 27e7250

Please sign in to comment.