chore(trace-query-builder): Improve the DX around span search bars#104757
chore(trace-query-builder): Improve the DX around span search bars#104757nsdeschenes merged 17 commits intomasterfrom
Conversation
|
@sentry review |
static/app/views/dashboards/widgetBuilder/buildSteps/filterResultsStep/spansSearchBar.tsx
Show resolved
Hide resolved
229ab0d to
c7eb927
Compare
gggritso
left a comment
There was a problem hiding this comment.
Looks like a nice little cleanup! The only thing I'm confused about mostly is the code about secondary aliases that got removed, how does that work?
Overall, things are still pretty confusing, would be great to resolve a few ambiguities:
- "EAP" vs. "TraceItem". The names of the components and hooks use both, but it's not clear when or why. Are they synonymous? Are they different? If so, why?
- Hooks that provide props for query builders vs. for providers. Are the providers even needed? Do the hooks need to be external to the providers, or can the providers load their own data?
This looks like a good step though 👍🏻
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #104757 +/- ##
========================================
Coverage 80.41% 80.42%
========================================
Files 9393 9393
Lines 403388 403340 -48
Branches 25923 25922 -1
========================================
- Hits 324404 324367 -37
+ Misses 78544 78533 -11
Partials 440 440 |
There was a problem hiding this comment.
Bug: Props dropped: component ignores key configuration properties
The TraceItemSearchQueryBuilder component explicitly destructures its props but omits replaceRawSearchKeys, matchKeySuggestions, caseInsensitive, and onCaseInsensitiveClick. These props are defined in TraceItemSearchQueryBuilderProps and accepted by useTraceItemSearchQueryBuilderProps, but the component never extracts or passes them to the hook. When callers spread props like {...spanSearchQueryBuilderProps}, these configuration values are silently discarded, breaking features like raw search key replacement and case insensitivity toggles.
static/app/views/explore/components/traceItemSearchQueryBuilder.tsx#L155-L192
sentry/static/app/views/explore/components/traceItemSearchQueryBuilder.tsx
Lines 155 to 192 in 0a9d38a
Bug: Placeholder prop ignored in search query builder hook
The useTraceItemSearchQueryBuilderProps hook does not destructure the placeholder prop from its parameters. The type includes placeholder via the intersection with SpanSearchQueryBuilderProps, but the hook ignores it and always computes a default placeholder via itemTypeToDefaultPlaceholder(itemType). Multiple callers pass custom placeholder values like t('Search for span attributes') which are silently discarded, resulting in incorrect placeholder text.
static/app/views/explore/components/traceItemSearchQueryBuilder.tsx#L64-L86
0a9d38a to
a2c0bca
Compare
There was a problem hiding this comment.
Bug: Component drops props for case sensitivity and search features
The TraceItemSearchQueryBuilder component accepts TraceItemSearchQueryBuilderProps but its function signature omits caseInsensitive, onCaseInsensitiveClick, matchKeySuggestions, and replaceRawSearchKeys. These props are passed by callers (via spanSearchQueryBuilderProps from useSpanSearchQueryBuilderProps) but silently dropped, causing features like case-insensitive search toggle and key suggestions to not function. The props need to be destructured and passed to useTraceItemSearchQueryBuilderProps.
static/app/views/explore/components/traceItemSearchQueryBuilder.tsx#L155-L192
sentry/static/app/views/explore/components/traceItemSearchQueryBuilder.tsx
Lines 155 to 192 in a2c0bca
…rch query builder
847b59b to
1deab3b
Compare
There was a problem hiding this comment.
Bug: Props not forwarded to internal hook in component
The TraceItemSearchQueryBuilder component accepts caseInsensitive, matchKeySuggestions, onCaseInsensitiveClick, and replaceRawSearchKeys in its props type, but doesn't destructure them or pass them to useTraceItemSearchQueryBuilderProps. The hook expects and uses these props (lines 81-84), so when callers pass them to the component, they are silently ignored. This breaks case insensitivity toggle, raw search key replacement, and key suggestion features when using this component directly.
static/app/views/explore/components/traceItemSearchQueryBuilder.tsx#L155-L192
sentry/static/app/views/explore/components/traceItemSearchQueryBuilder.tsx
Lines 155 to 192 in 1deab3b
The goal of this PR is to try and improve the DX of using (what was) the EAP span search bar. I've gone through and gotten rid of that component in favor of a more flexible hook to provide props for both
SearchQueryBuilderProvider, and that will enable span searching with theTraceItemSearchQueryBuilder.I've also moved the fetching of number and string attributes inside of the
useEAPSpanSearchQueryBuilderProps, so it is not longer required to be passed into theuseEAPSpanSearchQueryBuilderPropshook. This is to simplify some up coming work around dynamically fetching these attributes based off of the users search string while entering a key in the search query builder.