-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: handle all null autocomplete aggregateValues #7059
base: main
Are you sure you want to change the base?
fix: handle all null autocomplete aggregateValues #7059
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.
❌ Changes requested. Reviewed everything up to 5bb3563 in 1 minute and 48 seconds
More details
- Looked at
33
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:309
- Draft comment:
Consider checking array elements for non-null values instead of only testing if the array is non-empty. An array filled with nulls will still return true. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:313
- Draft comment:
Remove the console.log used for debugging before merging to production. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:309
- Draft comment:
The array check returns true if non‐empty regardless of its contents. If arrays of nulls should be excluded, consider checking if at least one element is non-null (e.g. using obj.some(item => hasNonNullValues(item))). - Reason this comment was not posted:
Comment looked like it was already resolved.
4. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:318
- Draft comment:
The base case returns 'obj !== null', which treats undefined as a valid value. Use 'obj != null' to exclude both null and undefined if that’s the intended behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment raises a valid point about undefined values being treated differently than null values. In JavaScript,!= null
checks for both null and undefined, while!== null
only checks for null. Looking at the function's usage in the code, it's used to find non-null values in the payload from getAttributesValues. The distinction between null and undefined could matter here.
I don't have enough context about the payload structure from getAttributesValues to know if undefined values need to be handled differently than null values. The function seems to work as intended with the current implementation.
While the suggestion is technically correct about JavaScript behavior, without clear evidence that undefined values need to be treated the same as null values in this context, we can't be certain this change is necessary.
The comment should be deleted because while it makes a technically valid point, we don't have strong evidence that treating undefined values the same as null values is required or beneficial in this specific context.
5. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:343
- Draft comment:
Swapping the check with find(hasNonNullValues) improves nested null detection. Ensure that this behavior aligns with expectations, especially for cases like boolean false or arrays containing only nulls. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_QGZYwzRNDBnzsGGp
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on fd765f8 in 26 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:312
- Draft comment:
Good removal of debugging console.log. Ensure no other unintended debugging logs remain in production code. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. frontend/src/pages/TracesExplorer/Filter/filterUtils.ts:312
- Draft comment:
Good removal of the debug console.log. Make sure debug logs aren’t inadvertently left in production code. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_DfqyBGO0HdHQCLCe
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
The Traces Explorer page breaks when the autocomplete/attribute_values response contains only null values. This happens because the relatedValues key is an object where keys can be of any type, and the transformation logic fails when all values are null.
Example problematic response:
this PR handles nested such cases
Related Issues / PR's
https://signoz-team.slack.com/archives/C089D1B5516/p1738858580435309
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Fixes Traces Explorer crash by handling null autocomplete responses using
hasNonNullValues
infilterUtils.ts
.useGetAggregateValues
infilterUtils.ts
where Traces Explorer breaks if autocomplete response contains only null values.hasNonNullValues
function to check for non-null values in nested objects.useGetAggregateValues
to usehasNonNullValues
for determining valid payload values.This description was created by
for fd765f8. It will automatically update as commits are pushed.