ref(grouping): Simplify handling of variant name and exception data (#97458)

During grouping, we pass data from one component strategy to another by storing it in the grouping context. In that context, two values - the variant name and a blob of exception data - are only sometimes defined. Until recently, the `GroupingContext` class supported key-lookup using subscript notation (`context[some_key]`) the same way a dictionary would, but lacked the safer `.get()` method. We therefore currently initialize both values to `None`, to prevent raising a `KeyError` if the values haven't been set.

Now that `.get()` has been implemented, though, we don't need to do that. This PR therefore switches the way we access them to use `.get()`, and removes the default initialization. In order to enforce when `variant_name` is or isn't expected to be defined, it also extends the practice already followed in some strategies of asserting on its existence (or lack thereof). This ensures that if we ever make changes to the grouping code, we'll know right away if our assumptions have been violated.

Author: lobsterkatie

src/sentry/grouping/strategies/base.py

@@ -117,7 +117,6 @@ def __init__(self, strategy_config: StrategyConfiguration, event: Event):
         self.config = strategy_config
         self.event = event
         self._push_context_layer()
-        self["variant_name"] = None
 
     def __setitem__(self, key: str, value: ContextValue) -> None:
         # Add the key-value pair to the context layer at the top of the stack
@@ -182,12 +181,15 @@ def get_single_grouping_component(
         Invoke the delegate grouping strategy corresponding to the given interface, returning the
         grouping component for the variant set on the context.
         """
+        variant_name = self["variant_name"]
+        assert variant_name is not None
+
         components_by_variant = self._get_grouping_components_for_interface(
             interface, event=event, **kwargs
         )
 
         assert len(components_by_variant) == 1
-        return components_by_variant[self["variant_name"]]
+        return components_by_variant[variant_name]
 
     def _get_grouping_components_for_interface(
         self, interface: Interface, *, event: Event, **kwargs: Any
@@ -484,7 +486,7 @@ def call_with_variants(
     **kwargs: Any,
 ) -> ReturnedVariants:
     context = kwargs["context"]
-    incoming_variant_name = context["variant_name"]
+    incoming_variant_name = context.get("variant_name")
 
     if incoming_variant_name is not None:
         # For the case where the variant is already determined, we act as a delegate strategy. To

src/sentry/grouping/strategies/configurations.py

@@ -27,9 +27,6 @@
     ],
     delegates=["frame:v1", "stacktrace:v1", "single-exception:v1"],
     initial_context={
-        # This key in the context tells the system which variant should
-        # be produced.  TODO: phase this out.
-        "variant_name": None,
         # This is a flag that can be used by any delegate to respond to
         # a detected recursion.  This is currently used by the frame
         # strategy to disable itself.  Recursion is detected by the outer
@@ -40,8 +37,6 @@
         "normalize_message": True,
         # Platforms for which context line should be taken into account when grouping.
         "contextline_platforms": ("javascript", "node", "python", "php", "ruby"),
-        # Stacktrace is produced in the context of this exception
-        "exception_data": None,
     },
 )
 

src/sentry/grouping/strategies/message.py

@@ -72,15 +72,17 @@ def normalize_message_for_grouping(message: str, event: Event) -> str:
 def message_v1(
     interface: Message, event: Event, context: GroupingContext, **kwargs: Any
 ) -> ReturnedVariants:
+    variant_name = context["variant_name"]
+
     # This is true for all but our test config
     if context["normalize_message"]:
         raw_message = interface.message or interface.formatted or ""
         normalized = normalize_message_for_grouping(raw_message, event)
         hint = "stripped event-specific values" if raw_message != normalized else None
-        return {context["variant_name"]: MessageGroupingComponent(values=[normalized], hint=hint)}
+        return {variant_name: MessageGroupingComponent(values=[normalized], hint=hint)}
     else:
         return {
-            context["variant_name"]: MessageGroupingComponent(
+            variant_name: MessageGroupingComponent(
                 values=[interface.message or interface.formatted or ""],
             )
         }

src/sentry/grouping/strategies/newstyle.py

@@ -300,6 +300,8 @@ def frame(
 ) -> ReturnedVariants:
     frame = interface
     platform = frame.platform or event.platform
+    variant_name = context["variant_name"]
+    assert variant_name is not None
 
     # Safari throws [native code] frames in for calls like ``forEach``
     # whereas Chrome ignores these. Let's remove it from the hashing algo
@@ -371,7 +373,7 @@ def frame(
     if context["is_recursion"]:
         frame_component.update(contributes=False, hint="ignored due to recursion")
 
-    return {context["variant_name"]: frame_component}
+    return {variant_name: frame_component}
 
 
 def get_contextline_component(
@@ -409,7 +411,7 @@ def get_contextline_component(
 def stacktrace(
     interface: Stacktrace, event: Event, context: GroupingContext, **kwargs: Any
 ) -> ReturnedVariants:
-    assert context["variant_name"] is None
+    assert context.get("variant_name") is None
 
     return call_with_variants(
         _single_stacktrace_variant,
@@ -425,6 +427,7 @@ def _single_stacktrace_variant(
     stacktrace: Stacktrace, event: Event, context: GroupingContext, kwargs: dict[str, Any]
 ) -> ReturnedVariants:
     variant_name = context["variant_name"]
+    assert variant_name is not None
 
     frames = stacktrace.frames
 
@@ -467,7 +470,7 @@ def _single_stacktrace_variant(
         frame_components,
         raw_frames,
         event.platform,
-        exception_data=context["exception_data"],
+        exception_data=context.get("exception_data"),
     )
 
     # This context value is set by the grouping info endpoint, so that the frame order of the

src/sentry/grouping/strategies/security.py

@@ -29,8 +29,10 @@
 def expect_ct_v1(
     interface: ExpectCT, event: Event, context: GroupingContext, **kwargs: Any
 ) -> ReturnedVariants:
+    variant_name = context["variant_name"]
+
     return {
-        context["variant_name"]: ExpectCTGroupingComponent(
+        variant_name: ExpectCTGroupingComponent(
             values=[
                 SaltGroupingComponent(values=["expect-ct"]),
                 HostnameGroupingComponent(values=[interface.hostname]),
@@ -44,8 +46,10 @@ def expect_ct_v1(
 def expect_staple_v1(
     interface: ExpectStaple, event: Event, context: GroupingContext, **kwargs: Any
 ) -> ReturnedVariants:
+    variant_name = context["variant_name"]
+
     return {
-        context["variant_name"]: ExpectStapleGroupingComponent(
+        variant_name: ExpectStapleGroupingComponent(
             values=[
                 SaltGroupingComponent(values=["expect-staple"]),
                 HostnameGroupingComponent(values=[interface.hostname]),
@@ -59,8 +63,10 @@ def expect_staple_v1(
 def hpkp_v1(
     interface: Hpkp, event: Event, context: GroupingContext, **kwargs: Any
 ) -> ReturnedVariants:
+    variant_name = context["variant_name"]
+
     return {
-        context["variant_name"]: HPKPGroupingComponent(
+        variant_name: HPKPGroupingComponent(
             values=[
                 SaltGroupingComponent(values=["hpkp"]),
                 HostnameGroupingComponent(values=[interface.hostname]),
@@ -74,6 +80,8 @@ def hpkp_v1(
 def csp_v1(
     interface: Csp, event: Event, context: GroupingContext, **kwargs: Any
 ) -> ReturnedVariants:
+    variant_name = context["variant_name"]
+
     violation_component = ViolationGroupingComponent()
     uri_component = URIGroupingComponent()
 
@@ -89,7 +97,7 @@ def csp_v1(
         uri_component.update(values=[interface.normalized_blocked_uri])
 
     return {
-        context["variant_name"]: CSPGroupingComponent(
+        variant_name: CSPGroupingComponent(
             values=[
                 SaltGroupingComponent(values=[interface.effective_directive]),
                 violation_component,

src/sentry/grouping/strategies/template.py

@@ -24,6 +24,8 @@
 def template_v1(
     interface: Template, event: Event, context: GroupingContext, **kwargs: Any
 ) -> ReturnedVariants:
+    variant_name = context["variant_name"]
+
     filename_component = FilenameGroupingComponent()
     if interface.filename is not None:
         filename_component.update(values=[interface.filename])
@@ -33,7 +35,7 @@ def template_v1(
         context_line_component.update(values=[interface.context_line])
 
     return {
-        context["variant_name"]: TemplateGroupingComponent(
-            values=[filename_component, context_line_component]
+        variant_name: TemplateGroupingComponent(
+            values=[filename_component, context_line_component],
         )
     }