feat(eap): Add array type handling for trace item table#7750
feat(eap): Add array type handling for trace item table#7750noahsmartin wants to merge 7 commits intomasterfrom
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Inspect the first element of the array to select the appropriate proto array type (IntArray, DoubleArray, StrArray) instead of always using StrArray. Extract _convert_array to a module-level function with proper type annotations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lity
Move array conversion logic from trace_item_table into a shared
`convert_tagged_array_to_attribute_value` in common.py that handles
the actual ClickHouse format (JSON-encoded tagged values like
`'{"Int":"165682342"}'`) and converts directly to typed protobuf arrays.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Agent transcript: https://claudescope.sentry.dev/share/9M1GRtap6XkWAIn0O7ZMbmxHEAjpeDqD35o2ssYqUTA
…ON column access Add a proper JsonPath expression type to the Snuba DSL for ClickHouse JSON sub-column access, replacing the DangerousRawSQL workaround used for TYPE_ARRAY attributes. The new expression supports simple type casts (col.`path`::Type) and complex types (col.`path`.:`Type`). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Agent transcript: https://claudescope.sentry.dev/share/hZfVjwU6F2FYeVIh1WhhhaGXuyTAcMFKitmF1SFBY14
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
phacops
changed the title
…n for JSON column access" This reverts commit 9f3d5da.
phacops
changed the title
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.
| if tag == "Double": | ||
| return AttributeValue(val_double_array=DoubleArray(values=[float(v) for v in values])) | ||
| return AttributeValue(val_str_array=StrArray(values=values)) | ||
|
|
There was a problem hiding this comment.
Empty arrays converted to null
High Severity
convert_tagged_array_to_attribute_value returns AttributeValue(is_null=True) when elements is empty. ClickHouse JSON sub-paths commonly return empty arrays (including for missing keys per the PR description), so this changes “empty array” into “null”, breaking existence/emptiness semantics and potentially causing incorrect API responses.
| if tag == "Double": | ||
| return AttributeValue(val_double_array=DoubleArray(values=[float(v) for v in values])) | ||
| return AttributeValue(val_str_array=StrArray(values=values)) | ||
|
|
There was a problem hiding this comment.
Bool-tag arrays parsed as integers
Medium Severity
convert_tagged_array_to_attribute_value routes the "Bool" tag into val_int_array and applies int(v) to each element. If ClickHouse encodes booleans as strings like "true"/"false" (or other non-numeric variants), this throws at runtime or silently misrepresents boolean arrays as integer arrays.
| [(tag, _)] = parsed[0].items() | ||
| values = [p[tag] for p in parsed] |
There was a problem hiding this comment.
Bug: The code makes unsafe assumptions about the data structure from ClickHouse, which can lead to ValueError or KeyError if the data format is unexpected.
Severity: MEDIUM
Suggested Fix
Add defensive checks to validate the data structure. Before unpacking, verify that parsed[0] is a dictionary with exactly one item. When iterating through the parsed list, use p.get(tag) with a default value or check if the key exists to avoid KeyError on mismatched types within the array. Consider logging a warning or error when malformed data is encountered.
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/web/rpc/common/common.py#L97-L98
Potential issue: The function `convert_tagged_array_to_attribute_value` makes unsafe
assumptions about the structure of data returned from ClickHouse. The line `[(tag, _)] =
parsed[0].items()` assumes the first element's dictionary contains exactly one key-value
pair, which will raise a `ValueError` if the dictionary is empty or has multiple keys.
Subsequently, the list comprehension `[p[tag] for p in parsed]` assumes all other
elements in the array use the same key (`tag`) as the first element, which will raise a
`KeyError` if any element has a different type tag. While ClickHouse is expected to
provide a consistent format, data corruption or schema mismatches could trigger these
unhandled exceptions.
kylemumma
left a comment
kylemumma
left a comment
There was a problem hiding this comment.
I think it would be good to add validation logic to to the function
| """ | ||
| if not elements: | ||
| return AttributeValue(is_null=True) | ||
|
|
There was a problem hiding this comment.
I think we should add validation that its a tagged array: all(is_tagged_element(e) for e in arr)
| [(tag, _)] = parsed[0].items() | ||
| values = [p[tag] for p in parsed] |
| values = [p[tag] for p in parsed] | ||
|
|
||
| if tag in ("Int", "Bool"): | ||
| return AttributeValue(val_int_array=IntArray(values=[int(v) for v in values])) |
There was a problem hiding this comment.
we should also add validation that all elements are the same type, since we are making that assumption
3 participants
Summary
TYPE_ARRAYsupport to the trace item table resolver, converting ClickHouse JSON column array values into typed protobuf responses (IntArray,DoubleArray,StrArray){"Int": "123"}) into properly typed arraysconvert_tagged_array_to_attribute_valueutility for reuse across resolversis_nullfor empty arrays instead of defaulting to emptyStrArrayTest plan
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.