Skip to content

fix(attribute-values): Properly support boolean attributes#8000

Merged
phacops merged 5 commits into
masterfrom
wmak/fix/handle-boolean-attribute-values
Jun 16, 2026
Merged

fix(attribute-values): Properly support boolean attributes#8000
phacops merged 5 commits into
masterfrom
wmak/fix/handle-boolean-attribute-values

Conversation

@wmak

@wmak wmak commented Jun 4, 2026

Copy link
Copy Markdown
Member

Summary

The TraceItemAttributeValues RPC enumerates the distinct values an attribute
takes (used to populate filter-value autocomplete). It previously assumed every
attribute was a string, so querying a boolean attribute broke. This makes
the endpoint properly support booleans.

Changes

  • Existence check on the right column. Booleans live in attributes_bool,
    not attributes_string. A small _ATTRIBUTE_TYPE_TO_COLUMN map drives the
    has(...) check by attribute type; unsupported types raise a clear
    BadSnubaRPCRequestException. Adding more types later is a one-line map entry.
  • Substring match guarded to strings. value_substring_match compiles to a
    SQL LIKE, which only makes sense for strings — it now raises on non-string
    keys instead of producing a broken query.
  • Lowercase boolean values. The response proto's values field is
    repeated string, so booleans must be serialized as strings. They're returned
    as lowercase "true"/"false" — matching how booleans are stringified across
    the EAP filter API — so the returned values round-trip as filter inputs.
    Serializing via bool(value) also guards against ClickHouse returning 0/1.

Tests

  • test_boolean_case — enumerating a boolean attribute returns
    ["true", "false"] with the expected counts.
  • test_substring_match_on_boolean_rejected — substring match on a boolean is
    rejected.
  • test_unsupported_attribute_type_rejected — non-string/bool types are
    rejected.

- The endpoint allows attribute values but currently breaks because it
  makes some assumptions where the value would be a string. This fixes
  those cases so that we can use this endpoint for booleans
@wmak
wmak requested review from a team as code owners June 4, 2026 19:20

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 5928272. Configure here.

Comment thread snuba/web/rpc/v1/trace_item_attribute_values.py Outdated
Comment thread snuba/web/rpc/v1/trace_item_attribute_values.py Outdated
Comment thread snuba/web/rpc/v1/trace_item_attribute_values.py Outdated
wmak and others added 2 commits June 11, 2026 17:37
Clean up the boolean-attribute handling so this endpoint properly
supports booleans:

- Replace the duplicated string/bool existence-check branches with an
  _ATTRIBUTE_TYPE_TO_COLUMN map, so supporting more types later is a
  single map entry.
- Serialize boolean values as lowercase "true"/"false" instead of
  Python's "True"/"False". This matches how booleans are stringified
  across the EAP filter API, so returned values round-trip as filter
  inputs. Using bool(value) also guards against ClickHouse returning
  0/1 ints.
- Add tests for substring-match-on-boolean and unsupported-type
  rejection paths.

Co-Authored-By: Claude <noreply@anthropic.com>

Agent transcript: https://claudescope.sentry.dev/share/_s-xOkXMoHkYblOZj1F_Om7TSAa3RNtVZSRIKERI1NY
Comment thread snuba/web/rpc/v1/trace_item_attribute_values.py
@phacops phacops changed the title fix(attribute-values): Handle boolean attributes fix(attribute-values): Properly support boolean attributes Jun 16, 2026
The Sentry review bot flagged f.has() on the attributes_bool Map column
as a potential bug. Verified it actually works in ClickHouse (the
existence check correctly excludes items missing the key), so the
finding is a false positive — but mapContains is the correct, conventional
function for Map key existence used everywhere else in the codebase, and
HashBucketFunctionTransformer rewrites it for the bucketed string/float
maps too. Switch to it for consistency and robustness.

Add test_boolean_existence_check with a bool attribute present on only
some items, giving real coverage of the existence filter (the existing
test seeds sentry.is_segment on every item, so it never exercised it).

Co-Authored-By: Claude <noreply@anthropic.com>

Agent transcript: https://claudescope.sentry.dev/share/6-4z37eRB8njhxT5P2w26JW2W9u_jJGuQGxZhX0AMgM
@phacops

phacops commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Re: the Sentry review-bot findings about f.has() on the attributes_bool Map column (HIGH + MEDIUM) — I verified these and they're false positives: has(attributes_bool, key) does check key existence correctly in ClickHouse here, and missing-key rows are not miscounted as false.

To prove it I added test_boolean_existence_check, which sets a custom_flag bool on only 2 of 9 seeded items (one true, one false) and leaves it off the rest. With the original f.has() code the test passed with counts == [1, 1] — i.e. the 7 rows without the key were correctly excluded, not folded into false. (The pre-existing test_boolean_case couldn't catch this either way, since gen_item_message injects sentry.is_segment=True on every item.)

That said, I still switched the existence check from f.has() to f.mapContains() (ead4824): it's the correct/conventional function for Map columns used everywhere else in the RPC code, HashBucketFunctionTransformer rewrites it for the bucketed attributes_string/attributes_float maps as well as the un-bucketed attributes_bool, and it keeps these warnings from recurring. The new test now gives real coverage of the existence filter for both has and mapContains.

@phacops
phacops enabled auto-merge (squash) June 16, 2026 23:13
@phacops
phacops merged commit 3edac08 into master Jun 16, 2026
66 checks passed
@phacops
phacops deleted the wmak/fix/handle-boolean-attribute-values branch June 16, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants