[issue platform] Clean up dataclass/subclassing (ab)use in `GroupType` definitions
- Dominant language
- Python
- Stars
- 44.8k
- Forks
- 4.9k
- Avg merge
- 21h 10m
- Merged PRs (30d)
- 635
Description
TL;DR: It's confusing and kind of odd the way we use frozen data classes and classvars and subclassing to define grouptypes, and we should fix it.
Convo with Claude exploring how it's set up now and how we might refactor it:
### `GroupType` design discussion
Notes from a conversation reviewing [`src/sentry/issues/grouptype.py`](https://github.com/getsentry/sentry/blob/master/src/sentry/issues/grouptype.py). The short version: the current "class-as-namespace-of-constants" pattern is awkward, `@dataclass(frozen=True)` is effectively a no-op on the subclasses, and the more idiomatic shape would be a single frozen `GroupType` dataclass with many instances.
---
### Q1: Would it make sense to have `GroupType` be an abstract class?
**Conceptually yes, but it would be a no-op in practice.** Evidence:
- `GroupType` is **never instantiated anywhere in the codebase**. Every usage is `type[GroupType]` or class-attribute access like `SomeSubclass.slug`.
- It declares required class attributes with no defaults: `type_id`, `slug`, `description`, `category` (`grouptype.py:245-248`).
- `grouptype.py:283-288` runs `__init_subclass__` which validates `cls.category` against `GroupCategory` — that check would fail on `GroupType` itself if it were ever subclassed-but-not-overridden, so the class is already self-policing.
So:
- **Making it `abc.ABC`** would signal intent more clearly and let a type checker flag accidental direct use. That's the main benefit.
- **But** ABCs primarily protect against *instantiation*, and nothing here ever instantiates `GroupType()` anyway. The required `ClassVar` fields (with no defaults) plus the `__init_subclass__` validation already enforce the contract for the way the code actually uses it.
- A lighter alternative used elsewhere in the codebase is `typing.Protocol` or just marking it `ClassVar`-only — but neither would change runtime behavior here.
**Verdict:** it's a reasonable cleanup for documentation/intent, but it wouldn't catch any real bug today.
---
### Q2: The subclasses are marked as dataclasses but `GroupType` isn't. Does this make sense? What does `frozen=True` mean here?
**The dataclass decorator is effectively a no-op on every class here**, which is why the inconsistency hasn't caused problems.
The key fact: every field on `GroupType` and its subclasses is `ClassVar`. By design, `@dataclass` **ignores `ClassVar` fields entirely** — it does not generate `__init__`, `__repr__`, or `__eq__` parameters for them. So `@dataclass(frozen=True)` on a `ClassVar`-only class produces:
- An `__init__()` that takes no arguments.
- A `__setattr__` that raises `FrozenInstanceError` on instances — but **instances are never created**.
What `frozen=True` *normally* means: once an instance is constructed, you can't do `instance.field = value`. For these classes it would also prevent assignment to class-level attributes via an instance, but since nothing constructs an instance, this protection isn't actually exercised.
So in practice:
- The `@dataclass(frozen=True)` decorators are **stylistic/cargo-culted**. They signal "this is an immutable value-like thing" but provide no runtime safety, since the actual data lives in `ClassVar`s on the class, not on instances.
- `GroupType` not having it is harmless — the decorator wouldn't do anything on it either.
- If you wanted the "frozen" guarantee to mean something, you'd need to either (a) drop `ClassVar` so the fields became dataclass fields and the class became instance-based, or (b) freeze the *class* itself (via something like `__init_subclass__` setting `__setattr__`, or just using a `Final`-ish convention).
External `grouptype.py` files in `src/sentry/uptime/`, `src/sentry/monitors/`, `src/sentry/grouping/`, etc. follow the same `@dataclass(frozen=True)` pattern, so this is a consistent codebase convention even if its runtime effect is nil.
#### TL;DR for both
The pattern is "use a *class itself* as a struct of constants, never instantiate." `ClassVar` makes this work; the `abc` and `dataclass(frozen=True)` decorations would mainly be documentation. The real contract is enforced by `__init_subclass__` + missing-default `ClassVar`s.
---
### Q3: So should they *all* be abstract, then? Instead of being dataclasses?
Probably not — and the deeper answer is that the pattern itself is the awkward part. Two distinct things to separate:
#### Abstract ≠ "not meant to be instantiated"
`abc.ABC` flags a class as needing further implementation. The leaves (`PerformanceSlowDBQueryGroupType`, etc.) are **concrete** — every required attribute is filled in. They're "abstract" only in the colloquial sense of "we never bother to call `()` on them." Python doesn't really have a built-in for "concrete class meant to be used as a namespace, not instantiated." So:
- **Base `GroupType`**: reasonable candidate for `ABC` — it's genuinely incomplete (no `type_id`, no `slug`, etc.).
- **Subclasses**: not abstract in any technical sense. Marking them `ABC` would be wrong; marking them `@typing.final` would be more accurate (signals "this is the leaf, don't subclass further") but also doesn't address the "don't instantiate" question.
#### The real shape of the code is "instances pretending to be classes"
Every leaf is just a bag of constants — `type_id=1001, slug="...", description="..."` — and the code only ever reads those constants off the class object. That's the data shape of an **instance** of a single `GroupType` dataclass, not of a class hierarchy. A more idiomatic version would be roughly:
```python
@dataclass(frozen=True)
class GroupType:
type_id: int
slug: str
description: str
category: int
noise_config: NoiseConfig | None = None
# ... etc
PERFORMANCE_SLOW_DB_QUERY = GroupType(
type_id=1001,
slug="performance_slow_db_query",
...
)
registry.add(PERFORMANCE_SLOW_DB_QUERY)
```
In that version:
- `frozen=True` actually does something (prevents `PERFORMANCE_SLOW_DB_QUERY.slug = "x"`).
- No `ClassVar` shenanigans, no `__init_subclass__` magic.
- The registry holds *instances*, not `type[GroupType]`.
- "Abstract base" becomes irrelevant — there's only one class.
The only thing this loses is the ability to override methods per type (e.g., `should_detect_escalation`). Looking at `grouptype.py:296-335`, all the classmethods read `cls.` — none of the subclasses actually override the methods themselves, only the flags. So polymorphism isn't being used; the class hierarchy is doing zero work that a single dataclass + instances couldn't do.
#### Practical recommendation
- **Don't** make the leaves abstract — they're concrete, that's the wrong label.
- **Could** make the base `GroupType` an `ABC` for documentation, but it's largely cosmetic.
- **The biggest improvement** would be flipping the pattern: one frozen dataclass, many instances, registry holds instances. That's a real refactor (touches every `type[GroupType]` annotation and every `Subclass.field` access), but it would make `frozen=True` mean something, eliminate `ClassVar` everywhere, and remove the `__init_subclass__` registration hook in favor of explicit `registry.add(...)`. Worth doing if you're already cleaning this area up; not worth it just for tidiness.
---
### Scope of a hypothetical refactor
- Every `type[GroupType]` annotation across the codebase becomes `GroupType`.
- Every `SomeSubclass.field` access becomes `SOME_INSTANCE.field`.
- `__init_subclass__` auto-registration is replaced by explicit `registry.add(...)` calls (or a decorator).
- The `ReplayGroupTypeDefaults` mixin becomes a helper constructor or a set of default kwargs.
- External `grouptype.py` files in `src/sentry/uptime/`, `src/sentry/monitors/`, `src/sentry/grouping/`, `src/sentry/incidents/`, `src/sentry/autopilot/`, `src/sentry/processing_errors/`, `src/sentry/preprod/size_analysis/` all need the same treatment.
Contributor guide
Research direction
Start with src/sentry/issues/grouptype.py, then inventory the type[GroupType] annotations and subclass field accesses described in the issue. Review the external grouptype.py files under uptime, monitors, grouping, incidents, autopilot, processing_errors, and preprod/size_analysis; work is done only after the target design and migration scope are agreed across these areas.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100