feat(grouping): Store grouping-method-specific metadata in GroupHashMetadata

[lobsterkatie]

This is a follow-up to #80531 (which added a new hashing_metaadata field to the GroupHashMetadata table), adding code to actual store data in the new field.

[codecov]

Codecov Report

Attention: Patch coverage is 92.70833% with 7 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/sentry/grouping/ingest/grouphash_metadata.py	91.86%	2 Missing and 5 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #80534   +/-   ##
=======================================
  Coverage   78.42%   78.42%           
=======================================
  Files        7211     7211           
  Lines      319706   319795   +89     
  Branches    44004    44027   +23     
=======================================
+ Hits       250719   250800   +81     
- Misses      62599    62604    +5     
- Partials     6388     6391    +3     

[JoshFerge]

I believe the way to do this as written is to type narrow within the various if functions. e.g. on the first condition:

    if hash_basis == HashBasis.STACKTRACE:
        if not isinstance(
            contributing_component,
            StacktraceGroupingComponent
            | ExceptionGroupingComponent
            | ChainedExceptionGroupingComponent
            | ThreadsGroupingComponent,
        ):
            raise ValueError(
                "Contributing component is not a StacktraceGroupingComponent for stacktrace basis"
            )
        if not isinstance(contributing_variant, ComponentVariant):
            raise ValueError("Contributing variant is not a ComponentVariant for stacktrace basis")

        hashing_metadata = _get_stacktrace_hashing_metadata(
            contributing_variant, contributing_component
        )

I would argue for replacing conditional with polymorphism as an alternative, but understand that given existing grouping code, this way is easier.

[lobsterkatie]

Oh, yeah... something like this, probably?

class BaseHashBasis[
    VariantType: BaseVariant,
    ComponentType: BaseGroupingComponent[Any] | None,
    MetadataType: HashingMetadata,
]:
    name: str
    variants: dict[str, BaseVariant]
    contributing_variant: VariantType
    contributing_component: ComponentType | None

    def __init__(
        self,
        variants: dict[str, BaseVariant],
        contributing_variant: VariantType,
        contributing_component: ComponentType | None = None,
    ):
        self.variants = variants
        self.contributing_variant = contributing_variant
        self.contributing_component = contributing_component

        if contributing_component:
            self.subcomponents_by_id = {
                subcomponent.id: subcomponent for subcomponent in contributing_component.values
            }

    def get_hashing_metadata(self) -> HashingMetadata:
        raise NotImplementedError()


class TemplateHashBasis(
    BaseHashBasis[ComponentVariant, TemplateGroupingComponent, TemplateHashingMetadata]
):
    name: str = "template"

    def get_hashing_metadata(self):
        metadata: TemplateHashingMetadata = {}

        if self.subcomponents_by_id["filename"].values:
            metadata["template_name"] = self.subcomponents_by_id["filename"].values[0]
        if self.subcomponents_by_id["context-line"].values:
            metadata["template_context_line"] = self.subcomponents_by_id["context-line"].values[0]

        return metadata

def get_hash_basis_and_metadata(
    event: Event, project: Project, variants: dict[str, BaseVariant]
) -> tuple[HashBasis, HashingMetadata]:
    # Skipping some of the details below, but you get the idea
    contributing_variant = ...
    contributing_component = ...

    hash_basis, hash_basis_type = GROUPING_METHODS_BY_DESCRIPTION[method_description]

    hashing_metadata = hash_basis_type(
        variants, contributing_variant, contributing_component
    ).get_hashing_metadata()

    return hash_basis, hashing_metadata 

Would the typechecker be any happier with that, though? Specifically, wouldn't I still have to type contributing_variant and contributing_component as Any in order to pass them to the TemplateHashBasis/etc constructor?

[JoshFerge]

I was thinking moreso putting formatting methods on the variant / component classes, and aiming to remove hash-basis as its own enum if possible. i don't have time right now to go into details, so please feel free to ignore, or look at the type narrowing using isinstance. whatever's easiest and can help get this merged faster.

[JoshFerge]

the PR looks good. included one idea to fix some of the Any typing. Could in the future look to replace more of the conditional logic with methods on the various Variant and Component classes. I have not reviewed the snapshots, and will do so tomorrow to spot check outputs. it feels kind of weird that there's no regular tests of this, but i assume in grouping code we let snapshots cover things generally.

[lobsterkatie]

replace more of the conditional logic with methods on the various Variant and Component classes

The problem with putting it there is there's not a nice 1-on-1 correspondence between hash basis and either of those class families. This is from one of my attempts along the way:

VARIANT_AND_COMPONENT_TYPES_BY_HASH_BASIS = {
    HashBasis.STACKTRACE: (
        ComponentVariant,
        StacktraceGroupingComponent
        | ExceptionGroupingComponent
        | ChainedExceptionGroupingComponent
        | ThreadsGroupingComponent,
    ),
    HashBasis.MESSAGE: (
        ComponentVariant,
        MessageGroupingComponent | ExceptionGroupingComponent | ChainedExceptionGroupingComponent,
    ),
    HashBasis.FINGERPRINT: (
        CustomFingerprintVariant | SaltedComponentVariant,
        None,
    ),
    HashBasis.SECURITY_VIOLATION: (
        ComponentVariant,
        CSPGroupingComponent
        | ExpectCTGroupingComponent
        | ExpectStapleGroupingComponent
        | HPKPGroupingComponent,
    ),
    HashBasis.TEMPLATE: (ComponentVariant, TemplateGroupingComponent),
    HashBasis.CHECKSUM: (ChecksumVariant, None),
    HashBasis.FALLBACK: (BaseVariant, None),
}

You'd end up still needing conditionals, just in the various classes' methods, wouldn't you? Like, ComponentVariant would need to check to see which of stacktrace, message, violation, and template it was dealing with, and then we're back to where we started, no?

(Putting it on components has the same problem - ExceptionGroupingComponent, for example, would need to check if we're doing stacktrace or message hashing. Also, both CustomFingerprintVariant and SaltedCompoentVariant would need the code to get fingerprint metadata, so then does that one type of metadata-getter become a mixin, when none of the rest are? Any way you slice it, it's not exactly pretty...)