Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(grouping): Add types to enhancement methods #87370

Merged
merged 7 commits into from
Mar 20, 2025
+183 −61
Merged

ref(grouping): Add types to enhancement methods #87370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f91b871
add `EnhancementRuleDict` type
lobsterkatie Mar 20, 2025
d8e38c4
add `EnhancementsDict` type
lobsterkatie Mar 20, 2025
3bcdf2e
add `ReturnValueCache` type
lobsterkatie Mar 20, 2025
fb44d70
add `MatchFrame` and `MatchFrameKey` types
lobsterkatie Mar 20, 2025
4eafd28
type cache in enhancements test as `ReturnValueCache`
lobsterkatie Mar 20, 2025
8c59917
return `MatchFrame` instance from `create_match_frame`
lobsterkatie Mar 20, 2025
a637739
add types to enhancements modules
lobsterkatie Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions src/sentry/grouping/enhancer/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import zlib
from collections import Counter
from collections.abc import Sequence
from typing import Any, Literal
from typing import Any, Literal, NotRequired, TypedDict

import msgpack
import sentry_sdk
Expand All @@ -23,7 +23,7 @@
from .exceptions import InvalidEnhancerConfig
from .matchers import create_match_frame
from .parser import parse_enhancements
from .rules import EnhancementRule
from .rules import EnhancementRule, EnhancementRuleDict

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -131,14 +131,26 @@ def keep_profiling_rules(config: str) -> str:
return "\n".join(filtered_rules)


class EnhancementsDict(TypedDict):
id: str | None
bases: list[str]
latest: bool
rules: NotRequired[list[EnhancementRuleDict]]


class Enhancements:
# NOTE: You must add a version to ``VERSIONS`` any time attributes are added
# to this class, s.t. no enhancements lacking these attributes are loaded
# from cache.
# See ``GroupingConfigLoader._get_enhancements`` in src/sentry/grouping/api.py.

def __init__(
self, rules, rust_enhancements: RustEnhancements, version=None, bases=None, id=None
self,
rules: list[EnhancementRule],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use abstract types when within the signature (e.g. Sequence) and more specific ones when returning (e.g. list). Unless it gets on the way for whatever reason.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely not the hill I want to die on, and I can switch it if you feel strongly. But before I go through and make a bunch of changes:

In a purely theoretical sense I guess I understand the principle, but in practice, for methods used purely internally, it always feels like overkill to me. Is there a strong argument in favor that I'm missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't push hard on this. It is mostly as documented in the best practice document. I can understand the argument of internal usage.

rust_enhancements: RustEnhancements,
version: int | None = None,
bases: list[str] | None = None,
id: str | None = None,
):
self.id = id
self.rules = rules
Expand Down Expand Up @@ -279,8 +291,8 @@ def assemble_stacktrace_component(

return stacktrace_component

def as_dict(self, with_rules=False):
rv = {
def as_dict(self, with_rules: bool = False) -> EnhancementsDict:
rv: EnhancementsDict = {
"id": self.id,
"bases": self.bases,
"latest": projectoptions.lookup_well_known_key(
Expand All @@ -292,7 +304,8 @@ def as_dict(self, with_rules=False):
rv["rules"] = [x.as_dict() for x in self.rules]
return rv

def _to_config_structure(self):
def _to_config_structure(self) -> list[Any]:
# TODO: Can we switch this to a tuple so we can type it more exactly?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a named tuple?

        from collections import namedtuple
        
        ConfigStructure = namedtuple('ConfigStructure', ['version', 'bases', 'rules'])

        return ConfigStructure(
            version=self.version,
            bases=self.bases,
            rules=[x._to_config_structure(self.version) for x in self.rules],
        )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that'd work. I'd rather leave it for a future PR, though, as my goal with this one is purely to add types, not change behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

return [
self.version,
self.bases,
Expand All @@ -305,7 +318,11 @@ def dumps(self) -> str:
return base64.urlsafe_b64encode(compressed).decode("ascii").strip("=")

@classmethod
def _from_config_structure(cls, data, rust_enhancements: RustEnhancements) -> Enhancements:
def _from_config_structure(
cls,
data: list[Any],
rust_enhancements: RustEnhancements,
) -> Enhancements:
version, bases, rules = data
if version not in VERSIONS:
raise ValueError("Unknown version")
Expand All @@ -317,7 +334,7 @@ def _from_config_structure(cls, data, rust_enhancements: RustEnhancements) -> En
)

@classmethod
def loads(cls, data) -> Enhancements:
def loads(cls, data: str | bytes) -> Enhancements:
if isinstance(data, str):
data = data.encode("ascii", "ignore")
padded = data + b"=" * (4 - (len(data) % 4))
Expand All @@ -337,7 +354,9 @@ def loads(cls, data) -> Enhancements:

@classmethod
@sentry_sdk.tracing.trace
def from_config_string(cls, s, bases=None, id=None) -> Enhancements:
def from_config_string(
cls, s: str, bases: list[str] | None = None, id: str | None = None
) -> Enhancements:
rust_enhancements = parse_rust_enhancements("config_string", s)

rules = parse_enhancements(s)
Expand Down
42 changes: 28 additions & 14 deletions src/sentry/grouping/enhancer/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
from __future__ import annotations

from collections.abc import Callable, Sequence
from typing import Any
from typing import TYPE_CHECKING, Any

from sentry.grouping.component import BaseGroupingComponent
from sentry.grouping.enhancer.matchers import MatchFrame
from sentry.utils.safe import get_path, set_path

from .exceptions import InvalidEnhancerConfig

if TYPE_CHECKING:
from sentry.grouping.enhancer.rules import EnhancementRule

ACTIONS = ["group", "app"]
ACTION_BITSIZE = 8
# Ensure that the number of possible actions is smaller than the number of numbers which can be
Expand All @@ -30,14 +35,18 @@ class EnhancementAction:
def apply_modifications_to_frame(
self,
frames: Sequence[dict[str, Any]],
match_frames: Sequence[dict[str, Any]],
match_frames: list[MatchFrame],
idx: int,
rule: Any = None,
rule: Any | None = None,
) -> None:
pass

def update_frame_components_contributions(
self, components, frames: Sequence[dict[str, Any]], idx, rule=None
self,
components: list[BaseGroupingComponent],
frames: list[dict[str, Any]],
idx: int,
rule: Any | None = None,
) -> None:
pass

Expand All @@ -55,7 +64,7 @@ def is_updater(self) -> bool:
return self._is_updater

@classmethod
def _from_config_structure(cls, val, version: int):
def _from_config_structure(cls, val: list[str] | int, version: int) -> EnhancementAction:
if isinstance(val, list): # This is a `VarAction`
variable, value = val
return VarAction(variable, value)
Expand Down Expand Up @@ -88,7 +97,7 @@ def __str__(self) -> str:
self.key,
)

def _to_config_structure(self, version: int):
def _to_config_structure(self, version: int) -> int:
"""
Convert the action into an integer by
- converting the combination of its boolean value (if it's a `+app/+group` rule or a
Expand All @@ -100,7 +109,7 @@ def _to_config_structure(self, version: int):
"""
return ACTIONS.index(self.key) | (ACTION_FLAGS[self.flag, self.range] << ACTION_BITSIZE)

def _slice_to_range(self, seq, idx):
def _slice_to_range(self, seq: list[Any], idx: int) -> list[Any]:
if self.range is None:
return [seq[idx]]
elif self.range == "down":
Expand All @@ -122,17 +131,21 @@ def _in_app_changed(self, frame: dict[str, Any]) -> bool:
def apply_modifications_to_frame(
self,
frames: Sequence[dict[str, Any]],
match_frames: Sequence[dict[str, Any]],
match_frames: list[MatchFrame],
idx: int,
rule: Any = None,
rule: Any | None = None,
) -> None:
# Change a frame or many to be in_app
if self.key == "app":
for match_frame in self._slice_to_range(match_frames, idx):
match_frame["in_app"] = self.flag

def update_frame_components_contributions(
self, components, frames: Sequence[dict[str, Any]], idx, rule=None
self,
components: list[BaseGroupingComponent],
frames: list[dict[str, Any]],
idx: int,
rule: EnhancementRule | None = None,
) -> None:
rule_hint = "stack trace rule"
if rule:
Expand Down Expand Up @@ -184,19 +197,20 @@ def __init__(self, var: str, value: str) -> None:
def __str__(self) -> str:
return f"{self.var}={self.value}"

def _to_config_structure(self, version):
def _to_config_structure(self, version: int) -> list[str | int]:
# TODO: Can we switch this to a tuple so we can type it more exactly?
return [self.var, self.value]

def modify_stacktrace_state(self, state, rule):
def modify_stacktrace_state(self, state, rule) -> None:
if self.var not in VarAction._FRAME_VARIABLES:
state.set(self.var, self.value, rule)

def apply_modifications_to_frame(
self,
frames: Sequence[dict[str, Any]],
match_frames: Sequence[dict[str, Any]],
match_frames: list[MatchFrame],
idx: int,
rule: Any = None,
rule: Any | None = None,
) -> None:
if self.var == "category":
frame = frames[idx]
Expand Down
Loading
Loading