-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: overall improvements to span logs drawer empty state (i.e. trace logs empty state vs. span logs empty state + UI improvements) #9252
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: overall improvements to span logs drawer empty state (i.e. trace logs empty state vs. span logs empty state + UI improvements) #9252
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.
Important
Looks good to me! 👍
Reviewed everything up to 102bf04 in 1 minute and 43 seconds. Click for details.
- Reviewed
617lines of code in8files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/components/Logs/RawLogView/styles.ts:60
- Draft comment:
The refactored getCustomHighlightBackground now only accepts isHighlighted. Ensure this simplification (removing dark mode and logType variants) is intentional and that the design remains consistent. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. frontend/src/container/SpanDetailsDrawer/SpanLogs/useSpanContextLogs.ts:264
- Draft comment:
Calling reverse() directly on afterLogs mutates the array. Consider using a non-mutating approach (e.g. [...afterLogs].reverse()) to avoid side effects. - 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.
3. frontend/src/container/SpanDetailsDrawer/SpanRelatedSignals/SpanRelatedSignals.tsx:149
- Draft comment:
Avoid using inline styles. The Drawer component and closeIcon prop use inline style objects. Consider moving these styles to a CSS class or a styled component for consistency. - 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.
4. frontend/src/container/SpanDetailsDrawer/SpanRelatedSignals/SpanRelatedSignals.tsx:127
- Draft comment:
The API call fix described in issue #123 mentions missing URL params (e.g., service param), but the diff doesn’t show changes adding such parameters. Please verify that all required URL params are being passed to the API call. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that all required URL parameters are being passed to the API call. This falls under the rule of not asking the PR author to double-check things or ensure behavior is intended. Therefore, this comment should be removed.
5. frontend/src/container/SpanDetailsDrawer/SpanLogs/__tests__/SpanLogs.test.tsx:171
- Draft comment:
Typo: There's an extra space before the colon in the string "This could be because :". Consider changing it to "This could be because:". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the typo, it's an extremely minor formatting issue in a test file. The extra space doesn't affect functionality or readability in any meaningful way. The rules state not to make comments that are obvious or unimportant. This seems to fall into that category. The typo could be seen as a small inconsistency that might propagate if others copy this code. Also, fixing it would take minimal effort. The benefit of fixing this tiny spacing issue is far outweighed by the cognitive overhead of processing the comment and making the change. This is exactly the kind of nitpick that the rules tell us to avoid. Delete this comment as it points out an extremely minor formatting issue that doesn't meaningfully impact the code quality or readability.
Workflow ID: wflow_RstK0Ku4Sm11ZkXn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed 9b89314 in 2 minutes and 7 seconds. Click for details.
- Reviewed
188lines of code in3files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/container/SpanDetailsDrawer/SpanLogs/constants.ts:88
- Draft comment:
Changed operator from 'in' to '=' for trace-only filters. Ensure that the API expects a single equality (i.e. '='), and that this change is consistent with backend expectations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the API expects a single equality and that the change is consistent with backend expectations. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules. Therefore, this comment should be removed.
2. frontend/src/container/SpanDetailsDrawer/__tests__/SpanDetailsDrawer.test.tsx:161
- Draft comment:
Refactored apiCallHistory from an array to an object keyed by query type. This improves clarity for tracking individual query calls. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change that was made. It doesn't provide any actionable feedback or suggestions for improvement. According to the rules, purely informative comments should be removed.
3. frontend/src/container/SpanDetailsDrawer/__tests__/SpanDetailsDrawer.test.tsx:199
- Draft comment:
The condition checking for trace-only logs uses 'if (filterExpression.includes("trace_id ="))'. Given that span log filters also include 'trace_id =', ensure that the ordering of conditions reliably distinguishes trace-only queries from span queries. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. frontend/src/container/SpanDetailsDrawer/__tests__/SpanDetailsDrawer.test.tsx:459
- Draft comment:
The test for handling empty logs state has been removed. Please confirm that this behavior is either no longer needed or covered by other tests. - 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.
5. frontend/src/container/SpanDetailsDrawer/__tests__/mockData.ts:15
- Draft comment:
Updated timestamp comment: the mockSpan now uses milliseconds instead of microseconds. Verify that all consumers expect a millisecond timestamp. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify that all consumers expect a millisecond timestamp, which violates the rule against asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should not be approved.
6. frontend/src/container/SpanDetailsDrawer/__tests__/SpanDetailsDrawer.test.tsx:390
- Draft comment:
The PR description mentions fixing the API call to correctly pass URL params (including a 'service' parameter), but this diff doesn’t seem to address that. Please ensure that changes related to URL parameter propagation are implemented or adequately covered by tests. - 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_hzThG4E5jsTk3gf4
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
frontend/src/container/SpanDetailsDrawer/__tests__/SpanDetailsDrawer.test.tsx
Outdated
Show resolved
Hide resolved
…Drawer.test.tsx Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
This reverts commit da5331d.
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.
Important
Looks good to me! 👍
Reviewed 54f2ce2 in 1 minute and 3 seconds. Click for details.
- Reviewed
45lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/container/SpanDetailsDrawer/__tests__/SpanDetailsDrawer.test.tsx:277
- Draft comment:
The waitFor now combines checks for the overlay and log items. While this improves conciseness, consider splitting these into separate waitFor calls to help isolate which condition fails during debugging. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. frontend/src/container/SpanDetailsDrawer/__tests__/SpanDetailsDrawer.test.tsx:485
- Draft comment:
The test now waits for 3 API calls before rendering logs. Note that another test expects 4 API calls. Please ensure that the differing expectations (3 vs 4) are intentional and reflect the intended behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_eY3nxVp57KWIMKdu
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 1a73c4a in 1 minute and 0 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/container/SpanDetailsDrawer/__tests__/SpanDetailsDrawer.test.tsx:487
- Draft comment:
Updating the expectation from 3 to 4 API calls now accounts for the additional trace-only logs query. Please ensure that this extra call indeed passes all required (non-empty) URL parameters as per the issue requirements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that the extra API call passes all required URL parameters. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.
Workflow ID: wflow_NVVsKGwwpF56LBrd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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 improves the empty state handling for span logs in the trace viewer by distinguishing between scenarios where no logs exist for a specific span versus no logs existing for the entire trace. It introduces enhanced UI states and adds a fourth query to check for trace-level logs.
- Adds new
TRACE_ONLY_LOGSquery to check if any logs exist for the trace when span has no logs - Implements conditional empty states: simple message when span has no logs but trace does, enhanced state when entire trace has no logs
- Refactors
SpanLogscomponent to accept pre-fetched data as props instead of fetching internally
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/utils/logs.ts | Simplifies getCustomHighlightBackground by removing unused parameters |
| frontend/src/container/SpanDetailsDrawer/tests/mockData.ts | Updates timestamp units and adds trace-only filter test constant |
| frontend/src/container/SpanDetailsDrawer/tests/SpanDetailsDrawer.test.tsx | Updates tests to handle 4 API queries and refactors tracking structure |
| frontend/src/container/SpanDetailsDrawer/SpanRelatedSignals/SpanRelatedSignals.tsx | Moves log fetching logic up and passes data/config down to SpanLogs |
| frontend/src/container/SpanDetailsDrawer/SpanRelatedSignals/SpanRelatedSignals.styles.scss | Removes fixed width constraint on explorer button |
| frontend/src/container/SpanDetailsDrawer/SpanLogs/useSpanContextLogs.ts | Adds Phase 4 query for trace-only logs and returns hasTraceIdLogs flag |
| frontend/src/container/SpanDetailsDrawer/SpanLogs/constants.ts | Corrects filter operator from 'in' to '=' for trace_id |
| frontend/src/container/SpanDetailsDrawer/SpanLogs/tests/SpanLogs.test.tsx | Adds tests for both empty state scenarios |
| frontend/src/container/SpanDetailsDrawer/SpanLogs/SpanLogs.tsx | Refactors to receive data via props and conditionally render empty states |
| frontend/src/container/EmptyLogsSearch/EmptyLogsSearch.styles.scss | Adds light mode styles for empty state component |
| frontend/src/constants/reactQueryKeys.ts | Adds TRACE_ONLY_LOGS query key |
| frontend/src/components/Logs/RawLogView/styles.ts | Updates function call to match simplified signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
When logs for trace is available but not for selected span change it to
"No logs found for selected span. View logs for the current trace"
Change button to "View Logs"
Done, @nityanandagohain
|

📄 Summary
This PR introduces overall improvements to the span logs drawer’s empty state. It differentiates between the trace logs empty state and the span logs empty state for clearer user experience.
🔍 Related Issues
Closes https://github.com/SigNoz/engineering-pod/issues/2979
📸 Screenshots / Screen Recording (if applicable / mandatory for UI related changes)
Before:
Screen.Recording.2025-10-05.154825.mp4
After:
Screen.Recording.2025-10-05.154902.mp4
📋 Checklist
Important
Improves span and trace logs empty state handling with enhanced UI and new query logic.
TRACE_ONLY_LOGSquery key inreactQueryKeys.tsfor handling trace-only logs.SpanLogs.tsxto render enhanced empty state when no logs are found for the entire trace usingEmptyLogsSearch.useSpanContextLogs.tsto check for trace-only logs when span logs are absent.SpanRelatedSignals.tsxto includeemptyStateConfigfor enhanced empty state rendering.SpanRelatedSignals.styles.scssfor better UI consistency.SpanLogs.test.tsxto verify enhanced empty state rendering.SpanDetailsDrawer.test.tsxto check for correct API queries and UI behavior.useSpanContextLogsimport inSpanLogs.tsx.This description was created by
for 1a73c4a. You can customize this summary. It will automatically update as commits are pushed.