Skip to content

Conversation

yanatha99
Copy link
Contributor

@yanatha99 yanatha99 commented Oct 15, 2025

Description

Multiple bug fixes and visual improvements for Trace Details page

  • Fix vertical alignment of cell content in SpanHierarchyTable to all be centered
  • Fix nested field filtering with shared applySpanFilters util
  • Replace hard-coded SpanHierarchyTable default height of 500px with auto when there are fewer entries and capped at 70vh when there are more entries
  • Fix duplicate ppl queries on page load or filter change
  • Fix double re-render on filter change

Screenshot

Trace Details page before fixes
trace-details-before-fixes

Trace Details page after fixes
trace-details-after-fixes

Testing the changes

  1. Go to details page of any trace
  2. In the "Timeline" tab validate that all the contents of each row in table are vertically centered and align with each other (none of them should be lower or higher than other content in the same row)
  3. Go to the "Meta data" tab of the right drawer and apply any filter from there
  4. Validate that the expected spans still show in table
  5. Validate that the page does not seem to "flicker" and re-render multiple times
  6. Open chrome (or whatever web browser) dev tools and look at network calls
  7. Validate that on page refresh only one ppl query is made
  8. Validate that when adding or removing filters only one ppl query is made

Changelog

  • fix: trace details bug fixes and visual improvements

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

opensearch-changeset-bot bot added a commit to yanatha99/OpenSearch-Dashboards that referenced this pull request Oct 15, 2025
Copy link

codecov bot commented Oct 15, 2025

Codecov Report

❌ Patch coverage is 96.42857% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.36%. Comparing base (26ae58e) to head (d997a02).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...traces/span_detail_tables/span_hierarchy_table.tsx 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10725      +/-   ##
==========================================
- Coverage   60.36%   60.36%   -0.01%     
==========================================
  Files        4463     4464       +1     
  Lines      119616   119615       -1     
  Branches    19775    19775              
==========================================
- Hits        72210    72209       -1     
  Misses      42398    42398              
  Partials     5008     5008              
Flag Coverage Δ
Linux_1 26.58% <ø> (ø)
Linux_2 38.82% <ø> (ø)
Linux_3 38.91% <ø> (+<0.01%) ⬆️
Linux_4 33.06% <96.42%> (-0.01%) ⬇️
Windows_1 26.59% <ø> (ø)
Windows_2 38.79% <ø> (ø)
Windows_3 38.91% <ø> (ø)
Windows_4 33.06% <96.42%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@TackAdam TackAdam added the OSD Changes being merged by the OSD team label Oct 15, 2025
@TackAdam
Copy link
Collaborator

Overall it looks good, this is a great improvement reducing duplicate calls.
One blocking concern though:
The removed hidden column functionality on the Timeline. The user is able to switch tabs to get them back or refreshing but we don't want to cause confusion for first time experience if this is accidentally clicked.

HideColumnIssue.mov

The old behavior allowed the user to add it back
OldBehavior

@yanatha99 yanatha99 force-pushed the traces-timeline-waterfall branch from f9f4249 to d4b819f Compare October 15, 2025 21:08
@yanatha99
Copy link
Contributor Author

yanatha99 commented Oct 15, 2025

Overall it looks good, this is a great improvement reducing duplicate calls. One blocking concern though: The removed hidden column functionality on the Timeline. The user is able to switch tabs to get them back or refreshing but we don't want to cause confusion for first time experience if this is accidentally clicked.

HideColumnIssue.mov
The old behavior allowed the user to add it back OldBehavior

Thanks for the callout! Removed column actions from the SpanHierarchyTable all together now with fix: disable column actions for SpanHierarchyTable

@yanatha99
Copy link
Contributor Author

yanatha99 commented Oct 15, 2025

fix: Stop column widths resizing when selecting spans is from TackAdam added it to this PR as well

TackAdam
TackAdam previously approved these changes Oct 15, 2025
Copy link
Collaborator

@TackAdam TackAdam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing requested changes

ruchidh
ruchidh previously approved these changes Oct 15, 2025
@yanatha99 yanatha99 dismissed stale reviews from ruchidh and TackAdam via 79319c3 October 15, 2025 23:27
@yanatha99 yanatha99 force-pushed the traces-timeline-waterfall branch from d4b819f to 79319c3 Compare October 15, 2025 23:27
@yanatha99
Copy link
Contributor Author

yanatha99 commented Oct 15, 2025

Rebased changes from main and also fixed unit test to account for this change fix: disable column actions for SpanHierarchyTable. No other diffs between this change set and previously approved change set.

TackAdam
TackAdam previously approved these changes Oct 15, 2025
ruchidh
ruchidh previously approved these changes Oct 15, 2025
@yanatha99 yanatha99 dismissed stale reviews from ruchidh and TackAdam via d997a02 October 16, 2025 16:43
@yanatha99 yanatha99 force-pushed the traces-timeline-waterfall branch from 79319c3 to d997a02 Compare October 16, 2025 16:43
@yanatha99
Copy link
Contributor Author

yanatha99 commented Oct 16, 2025

Rebased from main and fixed merge conflicts with #10728.
No other diffs between this change set and previously approved change set.

Did find a bug when testing locally, not sure which commit from the rebase introduced it. Dismissing a filter still correctly sets the spanFilters state, but the table items don't update accordingly.
Can see from the gif in PR description that this was working after my changes, but one of the rebased commits breaks it.
Would rather just get this PR in now, so it doesn't keep churning and I have to keep fixing bugs introduced by other changes.

@TackAdam
Copy link
Collaborator

Rebased from main and fixed merge conflicts with #10728. No other diffs between this change set and previously approved change set.

Did find a bug when testing locally, not sure which commit from the rebase introduced it. Dismissing a filter still correctly sets the spanFilters state, but the table items don't update accordingly. Can see from the gif in PR description that this was working after my changes, but one of the rebased commits breaks it. Would rather just get this PR in now, so it doesn't keep churning and I have to keep fixing bugs introduced by other changes.

This can be fixed with adding the following to trace_view line 544

key={`span-panel-${visualizationKey}-${spanFilters.length}-${transformedHits.length}`}

This will cause the proper re-render when filter changes.
This can be a fast follow today/tomorrow.

@TackAdam TackAdam merged commit 464dfe2 into opensearch-project:main Oct 16, 2025
125 of 128 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

OSD Changes being merged by the OSD team repeat-contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants