[CLEAN] Synthetic Benchmark PR #103102 - feat(tracemetrics): Parse metric from aggregations

[tomerqodo]

User description

Benchmark PR getsentry#103102

Type: Clean (correct implementation)

Original PR Title: feat(tracemetrics): Parse metric from aggregations
Original PR Description: This parses the metric from aggregations instead of relying on the top level metric name/type. The top level ones still take precedence for better compatibility. It has the limitation that it only allows a single metric across all the aggregations.
Original PR URL: getsentry#103102

---

PR Type

Enhancement

---

Description

• Parse metric information from aggregation functions instead of relying solely on top-level metric configuration

• Extract metric name, type, and unit from function arguments and store in new ResolvedMetricAggregate class

• Refactor metric filter generation into reusable utility functions for cleaner code organization

• Add support for multiple aggregations with the same metric while preventing cross-metric aggregations

---

Diagram Walkthrough

flowchart LR
  A["Aggregation Functions"] -->|extract metric info| B["ResolvedMetricAggregate"]
  C["Top-level Config"] -->|fallback| B
  B -->|generate filters| D["Metric Filters"]
  D -->|apply to query| E["Query Execution"]

Loading

File Walkthrough

	Relevant files
Enhancement	columns.py Add metric extraction from aggregation arguments                  src/sentry/search/eap/columns.py Add cast import for type casting Import MetricType from types module Create new ResolvedMetricAggregate dataclass extending ResolvedAggregate with metric metadata fields Update TraceMetricAggregateDefinition.resolve() to extract and store metric name, type, and unit from function arguments Return ResolvedMetricAggregate instead of ResolvedAggregate with populated metric fields +24/-6    resolver.py Add column-aware query resolution with dataset conditions src/sentry/search/eap/resolver.py Import and_trace_item_filters utility function Refactor resolve_query() to use and_trace_item_filters() for cleaner filter combination logic Add new resolve_query_with_columns() method that accepts selected columns and equations Implement resolve_dataset_conditions() to extract dataset-specific conditions from selected columns Call config.extra_conditions() with column and equation information for metric-aware filtering +35/-22  config.py Implement metric parsing from aggregations with fallback  src/sentry/search/eap/trace_metrics/config.py Move MetricType definition to types module Create new Metric dataclass to represent metric with name, type, and unit Refactor extra_conditions() signature to accept selected columns and equations Add _extra_conditions_from_metric() to handle top-level config metric filtering Add _extra_conditions_from_columns() to parse metrics from aggregation functions Extract get_metric_filter() as standalone function for reusability Add validation to prevent aggregating multiple different metrics in single query +118/-37 types.py Update config interface and centralize type definitions    src/sentry/search/eap/types.py Update SearchResolverConfig.extra_conditions() signature to accept selected columns and equations parameters Move MetricType type alias definition from trace_metrics config to types module +9/-1
Refactoring	rpc_utils.py Extract filter combination utility function                            src/sentry/search/eap/rpc_utils.py Create new utility module with and_trace_item_filters() function Consolidate filter combination logic to handle multiple TraceItemFilter objects Return single filter, combined filter, or None based on input count +14/-0    rpc_dataset_common.py Integrate column-aware query resolution and consolidate utilities src/sentry/snuba/rpc_dataset_common.py Import and_trace_item_filters from rpc_utils module Update get_table_rpc_request() to call resolve_query_with_columns() with selected columns and equations Move variable declarations in get_timeseries_query() before filter resolution for proper metric extraction Remove manual extra_conditions calls and let resolve_query_with_columns() handle dataset conditions Remove duplicate and_trace_item_filters() function definition +18/-29  trace_metrics.py Remove redundant extra conditions extraction                          src/sentry/snuba/trace_metrics.py Remove manual extra_conditions extraction calls from run_table_query() and run_timeseries_query() Let resolve_query_with_columns() handle metric condition injection automatically +0/-6
Tests	test_organization_events_trace_metrics.py Update tests for metric unit format and aggregation parsing tests/snuba/api/endpoints/test_organization_events_trace_metrics.py Update test cases to use - instead of none for missing metric unit parameter Add three new test cases for metric extraction from aggregation functions Test single metric aggregation with embedded metric info Test multiple aggregations with same metric name Test error handling for multiple different metrics in single query +74/-11

---

[qodo-code-review]

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 query resolution paths and condition injections (e.g., resolve_query_with_columns and resolve_dataset_conditions) introduce critical behavior changes without any added auditing or logging of these actions. Referred Code def resolve_query_with_columns( self, querystring: str | None, selected_columns: list[str] | None, equations: list[str] | None, ) -> tuple[ TraceItemFilter | None, AggregationFilter | None, list[VirtualColumnDefinition | None], ]: where, having, contexts = self.resolve_query(querystring) # Some datasets like trace metrics require we inject additional # conditions in the top level. dataset_conditions = self.resolve_dataset_conditions(selected_columns, equations) where = and_trace_item_filters(where, dataset_conditions) return where, having, contexts 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: Input validation: Metric parsing trusts argument positions and only validates emptiness/type loosely, potentially leading to runtime issues if fewer than expected arguments are supplied or wrong types are passed. Referred Code metric_name = None metric_type = None metric_unit = None if all( isinstance(resolved_argument, str) and resolved_argument != "" for resolved_argument in resolved_arguments[1:] ): # a metric was passed metric_name = cast(str, resolved_arguments[1]) metric_type = cast(MetricType, resolved_arguments[2]) metric_unit = None if resolved_arguments[3] == "-" else cast(str, resolved_arguments[3]) elif all(resolved_argument == "" for resolved_argument in resolved_arguments[1:]): # no metrics were specified, assume we query all metrics pass else: raise InvalidSearchQuery( f"Trace metric aggregates expect the full metric to be specified, got name:{resolved_arguments[1]} type:{resolved_arguments[2]} unit:{resolved_arguments[3]}" ) 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: Set handling risks: Using a set of Metric dataclass instances (selected_metrics: set[Metric]) relies on dataclass hash/eq defaults which may not behave as intended for uniqueness and could mis-detect multiple metrics. Referred Code selected_metrics: set[Metric] = set() if selected_columns: stripped_columns = [column.strip() for column in selected_columns] for column in stripped_columns: match = fields.is_function(column) if not match: continue resolved_function, _ = search_resolver.resolve_function(column) if not isinstance(resolved_function, ResolvedMetricAggregate): continue if not resolved_function.metric_name or not resolved_function.metric_type: continue metric = Metric( metric_name=resolved_function.metric_name, metric_type=resolved_function.metric_type, metric_unit=resolved_function.metric_unit, ) ... (clipped 13 lines) 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]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Improve metric argument parsing logic Refine the metric argument parsing logic to be more flexible. Handle partially specified metrics and add validation for the metric_type to prevent potential errors. src/sentry/search/eap/columns.py [403-417] -if all( - isinstance(resolved_argument, str) and resolved_argument != "" - for resolved_argument in resolved_arguments[1:] -): - # a metric was passed - metric_name = cast(str, resolved_arguments[1]) - metric_type = cast(MetricType, resolved_arguments[2]) - metric_unit = None if resolved_arguments[3] == "-" else cast(str, resolved_arguments[3]) -elif all(resolved_argument == "" for resolved_argument in resolved_arguments[1:]): - # no metrics were specified, assume we query all metrics - pass -else: - raise InvalidSearchQuery( - f"Trace metric aggregates expect the full metric to be specified, got name:{resolved_arguments[1]} type:{resolved_arguments[2]} unit:{resolved_arguments[3]}" - ) +metric_name = cast(str, resolved_arguments[1]) if resolved_arguments[1] else None +metric_type_str = cast(str, resolved_arguments[2]) if resolved_arguments[2] else None +metric_unit = cast(str, resolved_arguments[3]) if resolved_arguments[3] not in ("-", "") else None +metric_type: MetricType | None = None +if metric_type_str: + if metric_type_str not in get_args(MetricType): + raise InvalidSearchQuery(f"Invalid metric type: {metric_type_str}") + metric_type = cast(MetricType, metric_type_str) + +if metric_name and not metric_type: + raise InvalidSearchQuery("metric type is required when a metric name is provided") + Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the current logic is too strict and proposes a more flexible approach for parsing metric arguments, including validation for metric_type. This improves robustness and user experience by allowing partially specified metrics.	Medium
More

[tomerqodo]

Code Review by Qodo (Alpha)

Analysis Unavailable

Agentic review feature disabled, Please enable.

[qodo-code-review]

Code Review by Qodo (Alpha)

Come back soon, analysis in progress

[tomerqodo]

Code Review by Qodo (Alpha)

Automated review surfaced additional insights for this pull request.

💡 Suggestions (0) 🐞 Bugs (3) ⚠️ Rule Violations (3) 📋 Requirements Gap (0)

Issue Findings

● Action Required (1)

① Missing Dataset Conditions for Trace Metrics 🐞 Bugs

_Description

correctness ⚠️ Action Required ⚙ Source: 🐞 Bugs

• The endpoint `organization_trace_item_attributes.py` supports trace metrics queries but calls
`resolve_query()` instead of `resolve_query_with_columns()`
• This means trace metrics queries won't have the required metric name/type/unit filters applied at
the top level
• The endpoint uses `SearchResolverConfig()` instead of `TraceMetricsSearchResolverConfig`, which
would normally provide these conditions
• This could cause queries to return incorrect results by not filtering to the specific metric being
queried

_Evidence

The endpoint supports trace metrics (lines 148-149, 159-160, 170-171) but uses resolve_query() which
doesn't apply dataset-specific conditions. The new resolve_query_with_columns() method was
introduced specifically to inject these conditions for datasets like trace metrics.

_Referenced:
File Reference: src/sentry/api/endpoints/organization_trace_item_attributes.py[148-149]
File Reference: src/sentry/search/eap/resolver.py[163-170]

_{Code Reference}

@@ -250,7 +250,7 @@
        resolver = SearchResolver(
            params=snuba_params, config=SearchResolverConfig(), definitions=column_definitions
        )
-        query_filter, _, _ = resolver.resolve_query(query_string)
+        query_filter, _, _ = resolver.resolve_query(query_string)  # Should use resolve_query_with_columns
        meta = resolver.resolve_meta(referrer=referrer.value)
        meta.trace_item_type = constants.SUPPORTED_TRACE_ITEM_TYPE_MAP.get(
            trace_item_type, ProtoTraceItemType.TRACE_ITEM_TYPE_SPAN

↗ src/sentry/api/endpoints/organization_trace_item_attributes.py[253]

_{Remediation prompt}

🤖 Agent Prompt

## Issue Description
The endpoint `organization_trace_item_attributes.py` supports trace metrics queries but doesn't apply dataset-specific conditions because it calls `resolve_query()` instead of `resolve_query_with_columns()`.

## Issue Context
The PR introduced `resolve_query_with_columns()` to inject dataset-specific conditions (like metric name/type/unit filters for trace metrics). Endpoints that query trace metrics need to use this new method.

## Fix Focus Areas
- src/sentry/api/endpoints/organization_trace_item_attributes.py[253-253]

Change the call from `resolve_query(query_string)` to `resolve_query_with_columns(query_string, selected_columns, equations)`. You'll need to extract or determine the appropriate selected_columns and equations parameters from the request context.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

◐ Remediation recommended (4)

② Generic Variable Names Used ⚠️ Rule Violations

_Description

correctness ◐ Remediation Recommended ⚙ Source: ⚠️ Rule Violations

• The variable name `f` in the list comprehension is too generic and does not clearly express its
purpose
• The variable represents a `TraceItemFilter` object, but the single-letter name obscures this
meaning
• This violates the meaningful naming requirement, making the code less self-documenting

_Evidence

The compliance checklist requires that all identifiers clearly express their purpose and prohibits
single-letter variables except for conventional short-lived iterators. The variable `f` in the list
comprehension at rpc_utils.py:7 is a single-letter variable that represents a TraceItemFilter
object, which is not a conventional iterator context like `i` in a numeric loop.

_Referenced:
Rule 2: Generic: Meaningful Naming and Self-Documenting Code
File Reference: src/sentry/search/eap/rpc_utils.py[7-7]

_{Code Reference}

@@ -0,0 +7,1 @@
+    filters: list[TraceItemFilter] = [f for f in trace_item_filters if f is not None]

↗ src/sentry/search/eap/rpc_utils.py[7]

_{Remediation prompt}

🤖 Agent Prompt

## Issue description
The variable `f` in the list comprehension is too generic and does not clearly express that it represents a TraceItemFilter object.

## Issue Context
The code is filtering out None values from a list of TraceItemFilter objects. Using a single-letter variable name makes the code less readable and violates the meaningful naming compliance requirement.

## Fix Focus Areas
- src/sentry/search/eap/rpc_utils.py[7-7]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

③ Missing Error Context in ValueError ⚠️ Rule Violations

_Description

reliability ◐ Remediation Recommended ⚙ Source: ⚠️ Rule Violations

• The `ValueError` exceptions raised in `get_metric_filter` function lack sufficient context about
what caused the failure
• Error messages only state inability to resolve but don't include details about the metric being
processed or the search resolver state
• This makes debugging difficult in production environments where these errors might occur

_Evidence

The compliance checklist requires that error messages provide actionable context about what failed
and why, including relevant variable values and operation context. The ValueError exceptions at
lines 111, 115, and 137 in config.py only provide generic messages without including the metric
name, type, or unit being processed, making it difficult to debug failures in production.

_Referenced:
Rule 3: Generic: Robust Error Handling and Edge Case Management
File Reference: src/sentry/search/eap/trace_metrics/config.py[111-111]
File Reference: src/sentry/search/eap/trace_metrics/config.py[115-115]
File Reference: src/sentry/search/eap/trace_metrics/config.py[137-137]

_{Code Reference}

@@ -0,0 +111,1 @@
+        raise ValueError("Unable to resolve metric.name")

↗ src/sentry/search/eap/trace_metrics/config.py[111]

@@ -0,0 +115,1 @@
+        raise ValueError("Unable to resolve metric.type")

↗ src/sentry/search/eap/trace_metrics/config.py[115]

@@ -0,0 +137,1 @@
+            raise ValueError("Unable to resolve metric.unit")

↗ src/sentry/search/eap/trace_metrics/config.py[137]

_{Remediation prompt}

🤖 Agent Prompt

## Issue description
The ValueError exceptions lack sufficient context about what metric was being processed when the error occurred, making debugging difficult.

## Issue Context
The `get_metric_filter` function processes metric objects with name, type, and unit attributes. When resolution fails, the error should include these values to help identify the root cause.

## Fix Focus Areas
- src/sentry/search/eap/trace_metrics/config.py[111-111]
- src/sentry/search/eap/trace_metrics/config.py[115-115]
- src/sentry/search/eap/trace_metrics/config.py[137-137]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

④ Unsafe Type Casting Without Validation ⚠️ Rule Violations

_Description

reliability ◐ Remediation Recommended ⚙ Source: ⚠️ Rule Violations

• The code uses `cast()` to convert `resolved_arguments` to `str` and `MetricType` without
validating the actual types first
• If the arguments are not of the expected types, this could lead to runtime errors or incorrect
behavior
• The validation only checks if arguments are non-empty strings, but doesn't verify they match
expected types like valid MetricType values

_Evidence

The compliance checklist requires comprehensive error handling with validation of external inputs
and edge cases. The code at columns.py:408-410 uses type casting without validating that the
resolved_arguments actually contain valid MetricType values (counter, gauge, distribution). While it
checks for non-empty strings, it doesn't validate the metric_type is one of the allowed values.

_Referenced:
Rule 3: Generic: Robust Error Handling and Edge Case Management
File Reference: src/sentry/search/eap/columns.py[408-410]

_{Code Reference}

@@ -0,0 +408,3 @@
+            metric_name = cast(str, resolved_arguments[1])
+            metric_type = cast(MetricType, resolved_arguments[2])
+            metric_unit = None if resolved_arguments[3] == "-" else cast(str, resolved_arguments[3])

↗ src/sentry/search/eap/columns.py[408-410]

_{Remediation prompt}

🤖 Agent Prompt

## Issue description
The code casts resolved_arguments[2] to MetricType without validating it's a valid metric type value, which could lead to invalid data being processed.

## Issue Context
The MetricType is defined as a Literal type with allowed values: counter, gauge, distribution. The code should validate the argument matches one of these values before casting.

## Fix Focus Areas
- src/sentry/search/eap/columns.py[408-410]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

⑤ Equations Parameter Unused in Metric Extraction 🐞 Bugs

_Description

correctness ◐ Remediation Recommended ⚙ Source: 🐞 Bugs

• The `_extra_conditions_from_columns()` method accepts an `equations` parameter but never uses it
• Equations can contain metric aggregate functions (e.g., `sum(value,foo,counter,-) / count()`)
• If metric aggregates are only present in equations and not in selected_columns, the metric filters
won't be applied
• This could lead to incorrect query results when equations with metric aggregates are used

_Evidence

The equations parameter is passed through the call chain but never processed. Equations can contain
functions including metric aggregates, as shown in the resolver code. If equations are used with
metric aggregates, the metric information won't be extracted and filters won't be applied.

_Referenced:
File Reference: src/sentry/search/eap/resolver.py[1117-1121]
File Reference: src/sentry/search/eap/trace_metrics/config.py[54-87]

_{Code Reference}

@@ -47,10 +47,10 @@
    def _extra_conditions_from_columns(
        self,
        search_resolver: SearchResolver,
        selected_columns: list[str] | None,
-        equations: list[str] | None,
+        equations: list[str] | None,  # Parameter accepted but never used
    ) -> TraceItemFilter | None:
        selected_metrics: set[Metric] = set()

        if selected_columns:
            stripped_columns = [column.strip() for column in selected_columns]
@@ -70,6 +70,7 @@
                )
                selected_metrics.add(metric)

+        # Missing: Similar processing for equations parameter
        if not selected_metrics:
            return None

↗ src/sentry/search/eap/trace_metrics/config.py[50-87]

_{Remediation prompt}

🤖 Agent Prompt

## Issue Description
The `equations` parameter in `_extra_conditions_from_columns()` is accepted but never used. Equations can contain metric aggregate functions that need to be processed to extract metric information.

## Issue Context
Equations are arithmetic expressions that can contain functions, including metric aggregates. The current code only processes `selected_columns` to extract metric information.

## Fix Focus Areas
- src/sentry/search/eap/trace_metrics/config.py[50-87]

Add logic after processing selected_columns to also process equations. You'll need to:
1. Parse each equation to extract functions (similar to how selected_columns are processed)
2. Resolve each function and check if it's a ResolvedMetricAggregate
3. Extract metric information and add to selected_metrics set

Refer to how equations are parsed in `src/sentry/search/eap/resolver.py` lines 1097-1121 for guidance.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

○ Advisory comments (1)

⑥ Metric Unit Affects Deduplication Logic 🐞 Bugs

_Description

reliability ✧ Advisory ⚙ Source: 🐞 Bugs

• The `Metric` dataclass includes `metric_unit` in its equality and hash calculations
• If the same metric (name+type) is queried with different units, they will be treated as different
metrics
• This could trigger the "Cannot aggregate multiple metrics in 1 query" error even when querying the
same logical metric
• The behavior may be intentional if metrics should have consistent units, but could cause confusion

_Evidence

The Metric dataclass is frozen and includes all fields in equality checks. Test data shows metrics
can have different units (None vs 'millisecond'). If the same metric with different units is
queried, the set will contain multiple entries and trigger the validation error.

_Referenced:
File Reference: tests/snuba/api/endpoints/test_organization_events_trace_metrics.py[310-346]
File Reference: src/sentry/search/eap/trace_metrics/config.py[19-23]

_{Code Reference}

@@ -16,10 +16,10 @@


@dataclass(frozen=True, kw_only=True)
class Metric:
    metric_name: str
    metric_type: MetricType
-    metric_unit: str | None
+    metric_unit: str | None  # Included in equality/hash


@dataclass(frozen=True, kw_only=True)
@@ -77,10 +77,10 @@
                )
                selected_metrics.add(metric)

        if not selected_metrics:
            return None

        if len(selected_metrics) > 1:
-            raise InvalidSearchQuery("Cannot aggregate multiple metrics in 1 query.")
+            raise InvalidSearchQuery("Cannot aggregate multiple metrics in 1 query.")  # Triggers if units differ

↗ src/sentry/search/eap/trace_metrics/config.py[19-87]

_{Remediation prompt}

🤖 Agent Prompt

## Issue Description
The `Metric` dataclass includes `metric_unit` in equality checks, which means the same metric with different units will be treated as different metrics and could trigger validation errors.

## Issue Context
The validation at line 82 checks if more than one unique Metric exists in the set. Since metric_unit is part of equality, `Metric(name="foo", type="counter", unit="ms")` and `Metric(name="foo", type="counter", unit="seconds")` are different.

## Fix Focus Areas
- src/sentry/search/eap/trace_metrics/config.py[19-23]
- src/sentry/search/eap/trace_metrics/config.py[82-83]

Options:
1. Exclude metric_unit from equality by implementing custom `__eq__` and `__hash__` methods
2. Change validation to only check metric_name and metric_type for uniqueness
3. Document that this behavior is intentional if metrics must have consistent units

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---