-
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(FE): add sorting to the timestamp and duration column in trace explorer #7191
base: main
Are you sure you want to change the base?
Conversation
Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗 |
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 3acec72 in 2 minutes and 53 seconds
More details
- Looked at
107
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. frontend/src/container/TracesExplorer/ListView/utils.tsx:119
- Draft comment:
For duration columns, the cell now uses instead of formatting with getMs. Ensure this change is intended, as it no longer appends ‘ms’ or converts the duration value. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/src/container/TracesExplorer/ListView/utils.tsx:110
- Draft comment:
Avoid hardcoding color values. Replace 'magenta' with a design token or predefined color constant to maintain theming consistency. - Reason this comment was not posted:
Comment was on unchanged code.
3. frontend/src/container/TracesExplorer/ListView/utils.tsx:110
- Draft comment:
Avoid hardcoding the color 'magenta' in the Tag. Use a design token or predefined color constant to maintain consistent theming. - Reason this comment was not posted:
Comment was on unchanged code.
4. frontend/src/container/TracesExplorer/ListView/utils.tsx:94
- Draft comment:
The duration column sorter converts values using Number(a[key]). Ensure the values are always numeric or consider applying the same conversion as used in render (e.g. using getMs) for consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_SRgiODAEzkKadn6u
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.
@SagarRajput-7 please take a look |
Thanks for the PR @pranjalS9 . |
Can someone please check and merge PR please? |
Thanks for PR @pranjalS9 . We will review this PR soon. |
@pranjalS9 - current code and approach looks good, but this sorting is at the UI. We shouldn't implement local sorting for this table since it's paginated, and the data is fetched through API requests. Local sorting only makes sense when the client already has the entire dataset. To illustrate, imagine a service has 100k spans in the last 15 minutes. Since we can't display them all, we use pagination through the list API. If users want to see the slowest span, they're looking for the slowest span in the entire last 15 minutes, not just the slowest span among the 200 spans on the current page. Please see if we can implement server-side sorting I earlier worked on this - do take a look for reference - #5252 Thanks! |
Summary
Adds sorting to timestamp and duration columns in trace explorer in
utils.tsx
.Sorting Functionality:
Number(a.numericTimestamp) - Number(b.numericTimestamp)
ingetListColumns
inutils.tsx
wherenumericTimestamp
isnew Date(timestamp).getTime()
.Number(a[key]) - Number(b[key])
ingetListColumns
inutils.tsx
.Related Issues / PR's
Sorting missing in new traces explorer #5610
Issue link: #5610
Screenshots
Before - Without Sorting

After- With Sorting

Affected Areas and Manually Tested Areas
Trace Explorer table in List View