-
Notifications
You must be signed in to change notification settings - Fork 835
[OPIK-2160] Bayer - Traces - filter on specific input keys #2865
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements custom filtering functionality for traces, allowing users to filter on specific input and output keys. The feature enables filtering on nested paths within input/output data using JSONPath expressions with validation.
- Adds a new "Custom filter" column type that supports filtering on input/output keys
- Implements filter validation to ensure keys follow the correct format (input.[PATH] or output.[PATH])
- Extends the filtering system with error handling and validation capabilities
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
apps/opik-frontend/src/types/shared.ts | Adds COLUMN_CUSTOM_ID constant for the new custom filter column type |
apps/opik-frontend/src/types/filters.ts | Extends Filter interface with error field and FilterRowConfig with validation function |
apps/opik-frontend/src/lib/filters.ts | Updates filter validation logic to check for error conditions |
apps/opik-frontend/src/components/shared/FiltersButton/FilterRow.tsx | Adds filter validation and error display functionality |
apps/opik-frontend/src/components/shared/Autocomplete/Autocomplete.tsx | Replaces Input with DebounceInput component for better performance |
apps/opik-frontend/src/components/pages/TracesPage/TracesSpansTab/TracesSpansTab.tsx | Integrates custom filter column with validation for input/output key format |
apps/opik-frontend/src/components/pages-shared/traces/TraceDetailsPanel/TraceTreeViewer/helpers.ts | Implements filter transformation logic to convert custom filters to appropriate field/key combinations |
apps/opik-frontend/src/components/pages-shared/traces/TraceDetailsPanel/TraceTreeViewer/helpers.test.ts | Adds comprehensive test coverage for custom filter transformation and validation |
apps/opik-frontend/src/components/pages-shared/traces/TraceDetailsPanel/TraceDetailsActionsPanel.tsx | Adds custom filter support to trace details panel with dynamic key options |
filter.value && | ||
!/^((\$\.)?input|(\$\.)?output)(\.[^.]+)*$/.test(filter.key) | ||
) { | ||
return `Key is invalid, it should begin with "input", or "output" and follow this format: "input.[PATH]" For example: "input.message" `; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a spacing issue in the error message. There should be no space before the closing backtick and the period should be inside the quotes for consistency.
return `Key is invalid, it should begin with "input", or "output" and follow this format: "input.[PATH]" For example: "input.message" `; | |
return `Key is invalid, it should begin with "input", or "output" and follow this format: "input.[PATH]". For example: "input.message"`; |
Copilot uses AI. Check for mistakes.
const processFilter = (filterItem: Filter): Filter => { | ||
if (filterItem.field === COLUMN_CUSTOM_ID && filterItem.key) { | ||
const { field, key: originalKey, type: originalType } = filterItem; | ||
let key = originalKey; | ||
let type = originalType; | ||
let processedField: string = field; | ||
const prefixes = [ | ||
{ fieldName: "input" as const, prefix: "input." }, | ||
{ fieldName: "output" as const, prefix: "output." }, | ||
]; | ||
|
||
for (const { fieldName, prefix } of prefixes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefixes array could be defined as a constant outside the function to avoid recreating it on every function call, improving performance.
const processFilter = (filterItem: Filter): Filter => { | |
if (filterItem.field === COLUMN_CUSTOM_ID && filterItem.key) { | |
const { field, key: originalKey, type: originalType } = filterItem; | |
let key = originalKey; | |
let type = originalType; | |
let processedField: string = field; | |
const prefixes = [ | |
{ fieldName: "input" as const, prefix: "input." }, | |
{ fieldName: "output" as const, prefix: "output." }, | |
]; | |
for (const { fieldName, prefix } of prefixes) { | |
const PREFIXES = [ | |
{ fieldName: "input" as const, prefix: "input." }, | |
{ fieldName: "output" as const, prefix: "output." }, | |
]; | |
const processFilter = (filterItem: Filter): Filter => { | |
if (filterItem.field === COLUMN_CUSTOM_ID && filterItem.key) { | |
const { field, key: originalKey, type: originalType } = filterItem; | |
let key = originalKey; | |
let type = originalType; | |
let processedField: string = field; | |
for (const { fieldName, prefix } of PREFIXES) { |
Copilot uses AI. Check for mistakes.
processedField = fieldName; | ||
key = key.substring(prefix.length); | ||
type = COLUMN_TYPE.dictionary; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic assumes the first matching prefix wins, but if a key starts with 'input.output.something', it would incorrectly match 'input.' instead of being invalid. Consider validating that the key matches exactly one of the expected patterns.
processedField = fieldName; | |
key = key.substring(prefix.length); | |
type = COLUMN_TYPE.dictionary; | |
break; | |
const remainingKey = key.substring(prefix.length); | |
if (!remainingKey.includes(".")) { // Ensure no additional segments | |
processedField = fieldName; | |
key = remainingKey; | |
type = COLUMN_TYPE.dictionary; | |
break; | |
} |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job!
Details
Issues
Resolves #
Testing
Documentation