feat(eap): Resolve session_id and gen_ai.conversation.id to their indexed columns#8221
feat(eap): Resolve session_id and gen_ai.conversation.id to their indexed columns#8221pbhandari wants to merge 7 commits into
Conversation
…columns for which to use the index
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 74431a1. Configure here.
| base = field.parameters[0] | ||
| if isinstance(base, Column) and base.column_name in EMPTY_STRING_DEFAULT_COLUMNS: | ||
| return f.notEmpty(field) | ||
|
|
There was a problem hiding this comment.
Session exists filter always matches
Medium Severity
Registering sentry.session_id as a normalized column makes exists filters use isNotNull on a cast of the non-nullable session_id column, which is always true. Spans without the attribute in attributes_string used to fail an existence check; they now pass it even though the ingested column value is an auto-generated UUID.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 74431a1. Configure here.
| # so a bound equal to the mandatory range is byte-identical to it and gets | ||
| # collapsed by dedupe_timestamp_conditions. | ||
| if k.name == f"{COLUMN_PREFIX}timestamp" and value_type in ( | ||
| if k.name == sentry_column("timestamp") and value_type in ( |
There was a problem hiding this comment.
Null filters break conversation id
High Severity
Moving gen_ai.conversation.id off the map-backed filter path leaves null comparisons on the generic normalized-column logic. = null no longer matches spans whose unset ai_conversation_id is '', and != null adds an xor(isNull(...)) arm that is true for every row because the cast column is never NULL.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 74431a1. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| # ai_conversation_id when a TraceItem has no conversation_id. Because the column is never NULL, | ||
| # existence must be checked with notEmpty(...) instead of isNotNull(...), which would otherwise | ||
| # match every row (see get_field_existence_expression). | ||
| EMPTY_STRING_DEFAULT_COLUMNS: tuple[str, ...] = ("ai_conversation_id",) |
There was a problem hiding this comment.
Bug: The existence check for sentry.session_id incorrectly uses isNotNull. Since the column defaults to a zero-UUID instead of NULL, the check always returns true, matching all rows.
Severity: MEDIUM
Suggested Fix
The existence check for session_id should not use isNotNull. Instead, it should compare the column's value against the zero-UUID. A possible implementation is to generate a condition like notEquals(session_id, toUUID('00000000-0000-0000-0000-000000000000')). This requires modifying get_field_existence_expression to handle UUID columns with zero-UUID defaults specifically.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: snuba/protos/common.py#L198
Potential issue: The existence check for `sentry.session_id` is implemented using
`isNotNull(CAST(session_id, 'String'))`. However, the `session_id` column in the
database is a UUID type that defaults to the zero-UUID
(`'00000000-0000-0000-0000-000000000000'`) and is never NULL. Consequently, the
`isNotNull` check will always evaluate to true. This causes queries using
`exists(sentry.session_id)` to incorrectly return all rows, rather than only those with
a meaningful, non-default `session_id` value. The logic fails to account for the
zero-UUID default, unlike the handling for similar string columns.
Did we get this right? 👍 / 👎 to inform future reviews.
This reverts commit 925bfc9.
| tests/sentry/api/endpoints/test_organization_ai_conversations.py \ | ||
| tests/sentry/api/endpoints/test_organization_ai_conversation_details.py \ |
There was a problem hiding this comment.
New change: Adding the 2 failing CI tests on our CI.
| return AnyValue(string_value=value) | ||
| if isinstance(value, int): | ||
| return AnyValue(int_value=value) | ||
| if isinstance(value, bool): | ||
| return AnyValue(bool_value=value) | ||
| if isinstance(value, int): | ||
| return AnyValue(int_value=value) |
There was a problem hiding this comment.
new change. isintance(True, int) => True; so the bool case was never being exercised.
1 participant
This was previously implemented in - #8185, and had to be reverted due to CI failures. This also updates our CI to include the tests that failed in that run. Fixes for those tests are in getsentry/sentry#120608.
Description
sentry.session_idandgen_ai.conversation.idnow resolve to their dedicated top-level columns (session_id,ai_conversation_id) instead of theattributes_string[...]map, so=/IN <literal>filters on them prunegranules through the
bf_session_id/bf_ai_conversation_idbloom-filter skip indexes.The change is in the shared
attribute_key_to_expressionresolver, so every EAP endpoint (/events/TraceItemTable, TimeSeries, TraceItemStats, GetTraces, GetTrace, TraceItemDetails, ExportTraceItems) gets it automatically.To allow this, we refactored
NORMALIZED_COLUMNS_EAP_ITEMSso that it now maps an attribute name to an explicitNormalizedColumn(column_name, types)named-tuple rather than deriving the column by stripping thesentry.prefix. That is to facilitategen_ai.conversation.idto map toai_conversation_id.We also removed the
COLUMN_PREFIXconstant, as it's no longer serving it's original purpose. The lonesentry.timestampcheck that used it is inlined.Review focus — absence semantics: a missing value for used to read as
NULL(map miss) and now reads as the column default (''for the Stringai_conversation_id, the zero UUID forsession_id). This matches how existing normalized columns such assentry.trace_idalready behave. We expect the columns to already have been populated where appropriate.tests/web/rpc/v1/test_indexed_columns.pyruns each filter through a realTraceItemTablequery and asserts viaEXPLAIN indexes = 1that the expected skip index is applied, and that the returned rows are correct.Related PRs
Refs: https://linear.app/getsentry/issue/EAP-620
🤖 Generated with Claude Code