feat(transcripts): add span filtering with matched row highlighting#2202
feat(transcripts): add span filtering with matched row highlighting#2202
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / validate (pull_request).
|
0c317b3 to
1947fe5
Compare
| const TIME_RANGES: TimeRangeConfig[] = [ | ||
| { value: '5m', label: 'Last 5 minutes' }, | ||
| { value: '15m', label: 'Last 15 minutes' }, | ||
| { value: '30m', label: 'Last 30 minutes' }, | ||
| { value: '1h', label: 'Last 1 hour' }, | ||
| { value: '3h', label: 'Last 3 hours' }, | ||
| { value: '6h', label: 'Last 6 hours' }, | ||
| { value: '12h', label: 'Last 12 hours' }, | ||
| { value: '24h', label: 'Last 24 hours' }, | ||
| ]; |
There was a problem hiding this comment.
Let's use these packages for the transcripts UI feature in general:
- chrono (helps with natural language handling)
- little-date (handling selected timestamp intervals)
This should make sure we handle all dates/time properly in JS, and does not require date-fns migration. It's a quick win we can achieve to simplify the codebase.
| const hexToBytes = (hex: string): Uint8Array => { | ||
| const bytes = new Uint8Array(hex.length / 2); | ||
| for (let i = 0; i < hex.length; i += 2) { | ||
| bytes[i / 2] = Number.parseInt(hex.slice(i, i + 2), 16); | ||
| } | ||
| return bytes; | ||
| }; |
There was a problem hiding this comment.
You could use Buffer.from but I think that's available only in NodeJS 24+ so let's leave it
|
@claude review |
|
Claude finished @malinskibeniamin's task in 2m 16s —— View job Code Review Complete ✓I've reviewed the span filtering implementation across all changed files. Overall, this is a well-architected feature with clean separation of concerns. Here are my findings: 🎯 StrengthsExcellent Architecture
Modern React Patterns
UX Considerations
🔍 Issues & Suggestions1. Potential Performance Issue in Tree Filtering (span-filter.ts:24-48)The // Current: walks parent chain for each matched span
for (const spanId of matchedSpanIds) {
let current: string | undefined = spanId;
while (current && !needed.has(current) && current !== '0000000000000000') {
needed.add(current);
current = parentMap.get(current); // Could traverse deep trees repeatedly
}
}Suggestion: The early-exit check 2. Magic String for Root Span ID (span-filter.ts:41)while (current && !needed.has(current) && current !== '0000000000000000') {The hardcoded const ROOT_SPAN_ID = '0000000000000000';This makes intent clearer and prevents typos. 3. Missing Edge Case Test (span-filter.test.ts)The test suite is good but missing a test for circular references. While this shouldn't happen in valid OTEL data, defensive programming suggests testing it: it('handles circular parent references gracefully', () => {
const spans = [
createMockSpan('0000000000000001', '0000000000000002', 'span1'),
createMockSpan('0000000000000002', '0000000000000001', 'span2'), // circular!
];
// Should not infinite loop
const result = filterToMatchedAndAncestors(spans, new Set(['0000000000000001']));
expect(result).toHaveLength(2);
});4. Accessibility: Combobox Needs Explicit Label (transcript-filter-bar.tsx:230)<Combobox
creatable={true}
onChange={setAttributeKey}
options={ATTRIBUTE_OPTIONS}
value={attributeKey}
/>The Combobox is wrapped in a div with a Label, but there's no explicit connection. The Label should use <Label htmlFor="attr-key">Attribute</Label>
<Combobox id="attr-key" ... />Check if the Registry's Combobox component supports the 5. Type Safety Issue (transcript-filter-bar.tsx:459)onRemove={() => {
if (filter.id.startsWith('service-')) {
const serviceValue = filter.id.replace('service-', '');
// This could be an empty string if filter.id === 'service-'
onSelectedServicesChange(selectedServices.filter((s) => s !== serviceValue));If // Service value can be empty string (for "unknown" services)
const serviceValue = filter.id.replace('service-', '');6. Time Range Comment Reference (transcript-filter-bar.tsx:132)Per @malinskibeniamin's review comment, you should migrate to
This isn't blocking for this PR, but should be tracked for future work. 7. Potential Memory Leak (transcripts-table.tsx:514-540)The useEffect(() => {
// ...
}, [spanTree, expandedSpans.size]);If useEffect(() => {
const currentLength = spanTree.length;
if (prevSpanTreeLength.current === 0 && currentLength > 0 && expandedSpans.size === 0) {
// ...
}
prevSpanTreeLength.current = currentLength;
}, [spanTree]); // Remove expandedSpans.size8. Opacity Dimming Could Be More Selective (transcripts-table.tsx:378)// Dim unmatched parent spans
isUnmatchedParent && 'opacity-50'Reducing opacity to 50% affects all child elements including icons and text. Consider using isUnmatchedParent && 'text-muted-foreground/60 [&>*]:opacity-70'9. Proto Field Documentation (tracing.proto:213-219)The // Span IDs that matched the attribute filters, grouped by trace ID.
// Only populated when attribute_filters are active in the request.
// Use this to highlight matching spans in the UI or filter to partial traces client-side.
// Example: {"trace123": {"span_ids": ["span1", "span2"]}}
map<string, MatchedSpanIds> matched_spans = 7🐛 Minor Issues10. Unused Import (transcript-list-page.tsx)Check if all imports are used - the file is large and may have accumulated unused imports during refactoring. 11. Magic Number for Slow Threshold (transcript-list-page.tsx:172)const SLOW_THRESHOLD_NS = BigInt(5_000_000_000);Good that it's a constant! Consider adding a comment explaining why 5 seconds: // Slow threshold: 5 seconds in nanoseconds (user-facing "Slow (>5s)" filter)
const SLOW_THRESHOLD_NS = BigInt(5_000_000_000);📋 Test CoverageThe test file
Missing tests (nice-to-have):
🎨 Code QualityPositives:
Suggestions:
✅ RecommendationApprove with minor suggestions. The core implementation is solid and well-tested. The issues I've identified are mostly polish items that don't block merging:
The feature works as designed and follows the repo's established patterns. Great work on the comprehensive filtering system! |
Add MatchedSpanIds message and matched_spans field to ListTracesResponse to support server-side span filtering. This enables the frontend to highlight matched spans when filters are applied.
- Add filter bar with preset filters (LLM, tool, agent, error, slow) and custom attribute filters - Highlight matched spans with subtle muted background (distinct from blue selection color) - Support "show full traces" toggle to show all spans or only matched with ancestors - Dim unmatched parent spans when filters are active
- Add block statements to single-line returns (useBlockStatements) - Fix noLeakedRender by using ternary expressions - Eliminate nested ternaries by pre-computing chart query times - Apply lint autoformat to test file
… filter change - Replace fixed attribute dropdown with Combobox supporting custom input - Update attribute suggestions to valid OTel GenAI semantic conventions - Remove invalid attributes (trace.id, span.id, span.name, service.name) - Add comprehensive GenAI attributes (agent, request, response, tool, usage) - Add HTTP attributes for external tool calls - Clear span selection when filters change to prevent orphaned details panel
Add dedicated filter fields for service name and span ID filtering: - service_names: Filter traces by root service name (OR logic) - span_ids: Filter traces containing specific span IDs (OR logic) These fields use dedicated columns in the backend for efficient filtering, rather than requiring attribute filter syntax.
Add a new ServiceFilter component that allows filtering transcripts by service name: - Multi-select dropdown with checkboxes for each service - Shows service counts from currently loaded traces - Persists selection in URL state (?services=ai-agent) - Displays selected services as filter pills with remove buttons - Button remains visible but disabled during loading to prevent layout shift The filter uses the new service_names API field for efficient backend filtering. Traces without a service name (orphan traces) are excluded from the dropdown since they cannot be reliably filtered.
- Add CopyButton to conversation ID and trace ID fields - Display trace ID in overview tab - Convert base64 bytes fields to hex in raw JSON view - Add unit tests for hex-utils
- Extract ZERO_SPAN_ID constant for clarity - Single-pass lookup: build spanMap to avoid double bytesToHex calls - Construct result directly from Set using Map (avoids final filter) - Remove expandedSpans.size from useSpanExpansion deps (condition guards)
a8ce99c to
2c510d1
Compare
Add server-side span filtering with frontend highlighting for matched spans.