Skip to content

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025

User description

Benchmark PR getsentry#103425

Type: Clean (correct implementation)

Original PR Title: fix(grouping): Ensure custom titles use the correct frame
Original PR Description: This fixes a bug caused by getsentry#103268, which moved server-side fingerprinting before the call to normalize_stacktraces_for_grouping. While it's 99% true that they're unrelated processes (and therefore can happen in any order), the one exception to that is the handling of custom fingerprints which also include title information. In cases where the custom title includes any of the frame variables (function, module, package, or abs_path), the frame that gets used is the top in-app frame in the stacktrace. But in-app rules are applied as part of normalize_stacktraces_for_grouping, so having it not run until after the custom title is set is obviously a problem.

To fix this, the handling of such titles (in other words, the filling-in of the variables and adding of the result to the event) has been moved to live alongside the filling-in-of-the-variables which we do for the fingerprint itself, which is after in-app rules have been applied. A snapshot test illustrating the fix has also been added, showing that it's not the top frame but the top in-app frame which is used for the title.

(Why didn't I just switch the order of the two operations back, you ask? Because switching the order was the first step towards absorbing the normalization into variant calculation call which comes immediately after it, so server-side fingerprinting needed to get out of its original spot between them.)
Original PR URL: getsentry#103425


PR Type

Bug fix


Description

  • Move custom title resolution after stacktrace normalization

  • Ensure custom titles use correct in-app frame, not top frame

  • Add snapshot test validating title uses top in-app frame

  • Refactor title handling into dedicated function for clarity


Diagram Walkthrough

flowchart LR
  A["Normalize stacktraces<br/>for grouping"] --> B["Apply server-side<br/>fingerprinting"]
  B --> C["Resolve custom title<br/>with in-app frame"]
  C --> D["Event with correct<br/>custom title"]
Loading

File Walkthrough

Relevant files
Bug fix
api.py
Defer custom title resolution to after normalization         

src/sentry/grouping/api.py

  • Removed custom title handling from apply_server_side_fingerprinting
    function
  • Added new _apply_custom_title_if_needed function to resolve titles
    after normalization
  • Moved title resolution to get_grouping_variants_for_event after
    stacktrace normalization
  • Added import for get_path utility function
+17/-8   
Tests
__init__.py
Apply custom titles in test event creation                             

tests/sentry/grouping/init.py

  • Added imports for expand_title_template and get_path utilities
  • Updated _manually_save_event to apply custom titles after
    fingerprinting
  • Updated create_event to apply custom titles after fingerprinting
  • Added comments explaining when custom title handling occurs
+24/-1   
fingerprint-title-uses-top-in-app-frame.json
Add test input for custom title with in-app frame               

tests/sentry/grouping/fingerprint_inputs/fingerprint-title-uses-top-in-app-frame.json

  • New test input file with fingerprinting rule containing custom title
    template
  • Defines rule matching do_dog_stuff module with title using {{ function
    }} variable
  • Includes stacktrace with multiple frames to test in-app frame
    selection
+38/-0   
fingerprint_title_uses_top_in_app_frame.pysnap
Add snapshot test for in-app frame title resolution           

tests/sentry/grouping/snapshots/test_fingerprinting/test_event_hash_variant/fingerprint_title_uses_top_in_app_frame.pysnap

  • New snapshot test validating custom title resolution
  • Verifies title resolves to throw_ball (top in-app frame) not fetch
    (top frame)
  • Confirms custom fingerprint variant contributes and uses matched rule
+44/-0   

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: The new logic for applying custom titles and adjusting fingerprint-related metadata
performs critical grouping behavior without adding any auditing/logging, but the diff
alone cannot confirm whether higher-level layers capture sufficient audit trails.

Referred Code
# Check if the fingerprint includes a custom title, and if so, set the event's title accordingly.
_apply_custom_title_if_needed(fingerprint_info, event)

# Run all of the event-data-based grouping strategies. Any which apply will create grouping
# components, which will then be grouped into variants by variant type (system, app, default).
context = GroupingContext(config or _load_default_grouping_config(), event)
strategy_component_variants: dict[str, ComponentVariant] = _get_variants_from_strategies(
    event, context
)

# Create a separate container for these for now to preserve the typing of
# `strategy_component_variants`
additional_variants: dict[str, BaseVariant] = {}

# If the fingerprint is the default fingerprint, we can use the variants as is. If it's custom,
# we need to create a fingerprint variant and mark the existing variants as non-contributing.
# If it's hybrid, we'll replace the existing variants with "salted" versions which include
# the fingerprint.
if fingerprint_type == "custom":
    matched_rule = fingerprint_info.get("matched_rule")
    is_built_in_fingerprint = bool(matched_rule and matched_rule.get("is_builtin"))


 ... (clipped 46 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing guards: The new _apply_custom_title_if_needed function assumes a valid string template and event
structure without explicit error handling for malformed templates or missing event fields,
though such validation may occur elsewhere.

Referred Code
def _apply_custom_title_if_needed(fingerprint_info: FingerprintInfo, event: Event) -> None:
    """
    If the given event has a custom fingerprint which includes a title template, apply the custom
    title to the event.
    """
    custom_title_template = get_path(fingerprint_info, "matched_rule", "attributes", "title")

    if custom_title_template:
        resolved_title = expand_title_template(custom_title_template, event.data)
        event.data["title"] = resolved_title

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Template trust: Applying a custom title from fingerprint rule attributes to event data introduces derived
user-controlled content without explicit sanitization in this diff, though upstream
normalization may mitigate risks.

Referred Code
custom_title_template = get_path(fingerprint_info, "matched_rule", "attributes", "title")

if custom_title_template:
    resolved_title = expand_title_template(custom_title_template, event.data)
    event.data["title"] = resolved_title

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Reduce code duplication in tests

To reduce code duplication, replace the custom title logic in
_manually_save_event with a direct call to the _apply_custom_title_if_needed
function from sentry.grouping.api.

tests/sentry/grouping/init.py [76-114]

 def _manually_save_event(
     self, grouping_config: GroupingConfig, fingerprinting_config: FingerprintingConfig
 ) -> Event:
     """
     Manually complete the steps to save an event, in such a way as to not touch postgres (which
     makes it run a lot faster).
     """
     mgr = EventManager(data=self.data, grouping_config=grouping_config)
     mgr.normalize()
     data = mgr.get_data()
 
     # Before creating the event, manually run the parts of `EventManager.save` which are
     # necessary for grouping.
 
     normalize_stacktraces_for_grouping(data, load_grouping_config(grouping_config))
 
     data.setdefault("fingerprint", ["{{ default }}"])
     apply_server_side_fingerprinting(data, fingerprinting_config)
-    fingerprint_info = data.get("_fingerprint_info", {})
-    custom_title_template = get_path(fingerprint_info, "matched_rule", "attributes", "title")
 
     # Technically handling custom titles happens during grouping, not before it, but we're not
     # running grouping until later, and the title needs to be set before we get metadata below.
-    if custom_title_template:
-        resolved_title = expand_title_template(custom_title_template, data)
-        data["title"] = resolved_title
+    from sentry.grouping.api import _apply_custom_title_if_needed
+
+    _apply_custom_title_if_needed(data.get("_fingerprint_info", {}), data)
 
     event_type = get_event_type(data)
     event_metadata = event_type.get_metadata(data)
     data.update(materialize_metadata(data, event_type, event_metadata))
 
     return eventstore.backend.create_event(project_id=1, data=data)

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out code duplication in the test helper _manually_save_event and proposes a valid solution to centralize the logic by reusing the _apply_custom_title_if_needed function, which improves maintainability.

Medium
Refactor function to improve reusability

Refactor _apply_custom_title_if_needed to accept an event data dictionary
instead of an Event object to improve reusability and reduce code duplication in
tests.

src/sentry/grouping/api.py [458-467]

-def _apply_custom_title_if_needed(fingerprint_info: FingerprintInfo, event: Event) -> None:
+def _apply_custom_title_if_needed(
+    fingerprint_info: FingerprintInfo, data: MutableMapping[str, Any]
+) -> None:
     """
     If the given event has a custom fingerprint which includes a title template, apply the custom
-    title to the event.
+    title to the event data.
     """
     custom_title_template = get_path(fingerprint_info, "matched_rule", "attributes", "title")
 
     if custom_title_template:
-        resolved_title = expand_title_template(custom_title_template, event.data)
-        event.data["title"] = resolved_title
+        resolved_title = expand_title_template(custom_title_template, data)
+        data["title"] = resolved_title
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that changing the function signature to accept event data directly would allow for code reuse and eliminate duplication in the test files, improving maintainability.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants