Traces - Gantt chart / Span list rework #2257

TackAdam · 2024-11-18T22:06:01Z

Demo:

GanttChart.mp4

Description

Updates to gantt chart:

Added mini map above
Added reset button
Enabled zoom functionality
Added text to right displaying duration
Moved Pie chart above it. Fixed margins for text being cut off. Reformatted the display text.
Fixed resizing issue with side bar being opened.
Fixed text being cut off for left hand labels in the chart.
Enabled any clicks to the left of the bars to trigger service fly-out for selection.

Updates to Span list:

Added additional tab for "Tree view" to see hierarchy.
Fixed resizing issue with side bar being opened or page goin full screen

General fixes:

Made time changes and data source changes trigger a refresh for traces/services.
Made a helper function to replace the broken for EUI data grid fullscreen functionality.

Overall Look:

Tree View:

MiniMap and zoom:

Screen.Recording.2024-12-03.at.11.30.06.AM.mov

Resizing Fix:

Screen.Recording.2024-12-03.at.11.31.07.AM.mov

Issues Resolved

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…list nesting Signed-off-by: Adam Tackett <[email protected]>

Signed-off-by: Adam Tackett <[email protected]>

ps48 · 2024-11-26T23:53:44Z

Just a heads up here, while we are mostly getting in good shape with our Traces CI can you please add unit tests and cypress tests for these new workflows and bug fixed workflows?

Thanks!

Signed-off-by: Adam Tackett <[email protected]>

joshuali925 · 2024-12-03T23:28:17Z

public/components/trace_analytics/components/common/helper_functions.tsx

+  isFullScreen: boolean;
+}
+
+export const FullScreenWrapper: React.FC<FullScreenWrapperProps> = ({


could you add some comments above to explain what it does or why it's needed

Added above
//EUI Data grid full screen button is currently broken, this is a workaround using overlay mask

ps48 · 2024-12-04T00:11:17Z

public/components/trace_analytics/components/traces/traces_content.tsx

@@ -73,6 +73,10 @@ export function TracesContent(props: TracesProps) {
  const [includeMetrics, setIncludeMetrics] = useState(false);
  const isNavGroupEnabled = coreRefs?.chrome?.navGroup.getNavGroupEnabled();

+  useEffect(() => {


Can this useEffect be merged with the one on line number: 95 https://github.com/opensearch-project/dashboards-observability/pull/2257/files#diff-102a88265e317b8fd409fdb0aafb158beb0a68bec9b93f971e49ee8965fca62fR95-R122

Yes added the new dependencies to the use effect

startTime, endTime, props.dataSourceMDSId,

This triggers the refresh

ps48 · 2024-12-04T00:12:19Z

public/components/visualizations/plotly/plot.tsx

@@ -15,6 +15,8 @@ interface PltProps {
  onHoverHandler?: (event: Readonly<Plotly.PlotMouseEvent>) => void;
  onUnhoverHandler?: (event: Readonly<Plotly.PlotMouseEvent>) => void;
  onClickHandler?: (event: Readonly<Plotly.PlotMouseEvent>) => void;
+  onSelectedHandler?: (event: Readonly<Plotly.PlotSelectionEvent>) => void;
+  onRelayout?: (event: any) => void;


Should this be onRelayout?: (event: Readonly<Plotly.PlotSelectionEvent>) => void can refrin from using any as type.

ps48 · 2024-12-04T00:24:29Z

public/components/trace_analytics/components/traces/span_detail_table.tsx

+            showFullScreenSelector: false,
+            additionalControls: toolbarButtons,
+          }}
+          style={{ height: fullScreenMode ? '100%' : '500px', overflowY: 'auto' }}


Why is the datagrid restricted to 500px when not in fullscreen? Does this change on a bigger screen?

Auto can't be used as the window is initially rendered with the tree collapsed. So the window becomes roughly 50 pixels that is scrollable. 500pixels matches the limit of the "Span Tree" and keeps the window size for that panel consistent.

ps48 · 2024-12-04T00:30:05Z

public/components/trace_analytics/components/common/helper_functions.tsx

+          position: 'fixed',
+          top: 0,
+          left: 0,
+          width: '100vw',
+          height: '100vh',
+          backgroundColor: '#fff',
+          zIndex: 9999,
+          display: 'flex',
+          flexDirection: 'column',


Can add a class here and move the custom styling to index.scss

Added classes and move to scss file

.full-screen-wrapper { position: fixed; top: 0; left: 0; width: 100vw; height: 100vh; background-color: #fff; z-index: 9999; display: flex; flex-direction: column; } .full-screen-close-icon { position: absolute; top: 4px; right: 4px; z-index: 10000; } .full-screen-content { flex: 1 1 auto; overflow: auto; }

ps48 · 2024-12-04T00:31:36Z

public/components/trace_analytics/components/services/services_content.tsx

+  useEffect(() => {
+    refresh();
+  }, [startTime, endTime, props.dataSourceMDSId]);
+


Can we please merge this useEffect with the one on line : 83
https://github.com/opensearch-project/dashboards-observability/pull/2257/files#diff-6c41c2b6f0e21b1b29ad6d3cf28e92d0c5fc6bad2fbead527862b0634246ced8R83-R107

Yes added the new dependencies to the use effect

startTime, endTime, props.dataSourceMDSId,

This triggers the refresh

ps48 · 2024-12-04T00:48:47Z

public/components/trace_analytics/components/traces/__tests__/span_detail_panel.test.tsx

+  it('handles view toggle button group', () => {
+    const wrapper = mount(<SpanDetailPanel {...mockProps} />);
+    const toggleButtons = wrapper.find(EuiButtonGroup);
+    expect(toggleButtons).toHaveLength(1);
+
+    // Verify initial state is 'timeline'
+    expect(toggleButtons.prop('idSelected')).toBe('timeline');
+
+    // Simulate changing the toggle
+    act(() => {
+      toggleButtons.prop('onChange')!('span_list');
+    });
+
+    wrapper.update();
+
+    // Verify the toggle button group has been updated
+    const updatedToggleButtons = wrapper.find(EuiButtonGroup);
+    expect(updatedToggleButtons.prop('idSelected')).toBe('span_list');
+  });


For this test I feel we are testing the button and not what our page is rendering on button click. The button is already tested in OUI, we should rather test on button change is the rendered component changing to gantt-chart, table or tree view.

Removed the test as this functionality is covered by OUI and the table specific checks should be done in span_detail_table

paulstn · 2024-12-04T17:05:51Z

public/components/trace_analytics/components/traces/span_detail_table.tsx

+        </div>
+      );
+    case 'durationInNanos':
+      return `${round(nanoToMilliSec(Math.max(0, value)), 2)} ms`;


What's with the names here, where the column's id says 'nanos' but the function for the render seems to be showing milliseconds, where it calls a method named 'nanoToMilliSec'?

I kept that from the originally implementation which has been in place for 3 years, not sure the original reasoning.
dde9588#diff-491203dc294dcf7e9d988e5ba55b77471fccebc3e7d3d62c42dd0b0a6a365599R136

Signed-off-by: Adam Tackett <[email protected]>

ps48 · 2024-12-04T19:48:32Z

public/components/trace_analytics/components/traces/__tests__/span_detail_table.test.tsx

    await act(() => {
      ReactDOM.render(
-        <SpanDetailTable
+        <SpanDetailTableHierarchy
          http={httpClientMock}
          hiddenColumns={['traceId', 'traceGroup']}
          DSL={{}}
          openFlyout={(spanId: string) => setCurrentSpan(spanId)}
-          mode='data_prepper'
+          mode="data_prepper"
+          dataSourceMDSId="testDataSource"


Post snapshot check, we should add some tests to see if expand/collapse is working or not as that is the main functionality of this view.

Covered with added cypress testing as follows:

describe('Testing traces tree view',
it('Verifies tree view and table toggle functionality with expand/collapse logic',
it('Verifies tree view expand arrow functionality',
it('Verifies span flyout',
it('Handles toggling between full screen and regular modes',

ps48 · 2024-12-04T19:49:58Z

public/components/trace_analytics/components/traces/span_detail_panel.tsx

+    const updateAvailableWidth = () => {
+      if (containerRef.current) {
+        setAvailableWidth(containerRef.current.getBoundingClientRect().width);
+      } else {
+        setAvailableWidth(window.innerWidth);
+      }
+    };
+
+    const handleFullScreenChange = () => {
+      const isFullscreenActive = !!document.fullscreenElement;
+      setIsFullScreen(isFullscreenActive);
+      updateAvailableWidth();
+    };


Nit. I personally don't ride with adding the function definitions inside useEffect(). But is not a blocker.

ps48 · 2024-12-04T19:51:21Z

public/components/trace_analytics/components/traces/span_detail_panel.tsx

+  }, []);
+
+  const dynamicLayoutAdjustment = useMemo(() => {
+    return isLocked ? availableWidth - 350 : availableWidth - 150;


Should this change when new home is enabled/disabled? Is the pixel width of the sidebar constant with and without the new home?

Updated:

const dynamicLayoutAdjustment = useMemo(() => {
const newNavigation = coreRefs?.chrome?.navGroup.getNavGroupEnabled?.();
const adjustment = newNavigation ? 350 : 400;
return isLocked ? availableWidth - adjustment : availableWidth - 150;
}, [isLocked, availableWidth]);

ps48 · 2024-12-04T19:55:11Z

public/components/trace_analytics/components/traces/span_detail_panel.tsx

+        onRelayout={(event) => {
+          // Handle x-axis range update
+          if (event && event['xaxis.range[0]'] && event['xaxis.range[1]']) {
+            const newRange = [event['xaxis.range[0]'], event['xaxis.range[1]']];
+            setSelectedRange(newRange);
+          } else {
+            setSelectedRange(fullRange);
+          }
+        }}


Nit - not blocking. Just for good readability do you want to follow how other props functions are declared and used here. Something like onClick.

ps48 · 2024-12-04T20:20:35Z

public/components/trace_analytics/components/traces/span_detail_table.tsx

+  interface Span {
+    spanId: string;
+    parentSpanId?: string;
+    children: Span[];
+    [key: string]: any;
+  }
+
+  type SpanMap = Record<string, Span>;


nit. Not blocker if these are re-usable contants, we can move these to https://github.com/opensearch-project/dashboards-observability/blob/main/public/components/common/types.ts

Signed-off-by: Adam Tackett <[email protected]>

ps48

Can see the cypress tests passed! Thanks for the changes. The non-blocking changes can be done in the upcoming PRs.

opensearch-trigger-bot · 2024-12-06T17:37:52Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/dashboards-observability/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/dashboards-observability/backport-2.x
# Create a new branch
git switch --create backport/backport-2257-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ab396b94a14f922220e5ec071bc4f696208650b3
# Push it to GitHub
git push --set-upstream origin backport/backport-2257-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/dashboards-observability/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2257-to-2.x.

* fix refresh on timepicker change, update gannt chart, mini map, span list nesting Signed-off-by: Adam Tackett <[email protected]> * ui changes and new tab for spans Signed-off-by: Adam Tackett <[email protected]> * ui updates, additional bug fixes Signed-off-by: Adam Tackett <[email protected]> * add eui grid full screen, testing Signed-off-by: Adam Tackett <[email protected]> * add back table height for tree, cap left margin Signed-off-by: Adam Tackett <[email protected]> * add some jest testing Signed-off-by: Adam Tackett <[email protected]> * address comments Signed-off-by: Adam Tackett <[email protected]> * test Signed-off-by: Adam Tackett <[email protected]> * test remove Signed-off-by: Adam Tackett <[email protected]> * add cypress testing, conditional check for new nav Signed-off-by: Adam Tackett <[email protected]> * fix width of tables Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Co-authored-by: Adam Tackett <[email protected]> (cherry picked from commit ab396b9)

…ework (#2283) * Traces - Gantt chart / Span list rework (#2257) * fix refresh on timepicker change, update gannt chart, mini map, span list nesting Signed-off-by: Adam Tackett <[email protected]> * ui changes and new tab for spans Signed-off-by: Adam Tackett <[email protected]> * ui updates, additional bug fixes Signed-off-by: Adam Tackett <[email protected]> * add eui grid full screen, testing Signed-off-by: Adam Tackett <[email protected]> * add back table height for tree, cap left margin Signed-off-by: Adam Tackett <[email protected]> * add some jest testing Signed-off-by: Adam Tackett <[email protected]> * address comments Signed-off-by: Adam Tackett <[email protected]> * test Signed-off-by: Adam Tackett <[email protected]> * test remove Signed-off-by: Adam Tackett <[email protected]> * add cypress testing, conditional check for new nav Signed-off-by: Adam Tackett <[email protected]> * fix width of tables Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Co-authored-by: Adam Tackett <[email protected]> (cherry picked from commit ab396b9) * fix loaddash error, update snapshots Signed-off-by: Adam Tackett <[email protected]> * add missing format change Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Co-authored-by: Adam Tackett <[email protected]>

Adam Tackett and others added 3 commits November 18, 2024 14:03

fix refresh on timepicker change, update gannt chart, mini map, span …

6f71edb

…list nesting Signed-off-by: Adam Tackett <[email protected]>

ui changes and new tab for spans

46a3a76

Signed-off-by: Adam Tackett <[email protected]>

Merge branch 'main' into ganttChartUpdate

933c971

TackAdam added enhancement New feature or request backport 2.x mds-2.18.0 labels Nov 22, 2024

ui updates, additional bug fixes

1d17813

Signed-off-by: Adam Tackett <[email protected]>

TackAdam and others added 2 commits November 27, 2024 16:02

Merge branch 'main' into ganttChartUpdate

e0d2f05

add eui grid full screen, testing

f009ca5

Signed-off-by: Adam Tackett <[email protected]>

TackAdam changed the title ~~[WIP][DRAFT] Gantt chart / Span list updates~~ Traces - Gantt chart / Span list rework Dec 2, 2024

Adam Tackett added 2 commits December 2, 2024 15:58

add back table height for tree, cap left margin

22f20fe

Signed-off-by: Adam Tackett <[email protected]>

add some jest testing

7f361b1

Signed-off-by: Adam Tackett <[email protected]>

TackAdam marked this pull request as ready for review December 3, 2024 21:36

joshuali925 approved these changes Dec 3, 2024

View reviewed changes

ps48 reviewed Dec 4, 2024

View reviewed changes

paulstn reviewed Dec 4, 2024

View reviewed changes

address comments

456e9fd

Signed-off-by: Adam Tackett <[email protected]>

ps48 reviewed Dec 4, 2024

View reviewed changes

Adam Tackett added 2 commits December 4, 2024 16:10

test

7b5a9a2

Signed-off-by: Adam Tackett <[email protected]>

test remove

65a6906

Signed-off-by: Adam Tackett <[email protected]>

TackAdam and others added 4 commits December 5, 2024 09:21

Merge branch 'main' into ganttChartUpdate

74cd6da

add cypress testing, conditional check for new nav

f82741e

Signed-off-by: Adam Tackett <[email protected]>

Merge branch 'main' into ganttChartUpdate

c5c2372

fix width of tables

ecf88da

Signed-off-by: Adam Tackett <[email protected]>

ps48 approved these changes Dec 5, 2024

View reviewed changes

TackAdam merged commit ab396b9 into opensearch-project:main Dec 5, 2024
14 of 18 checks passed

opensearch-trigger-bot bot added the backport-failed label Dec 5, 2024

TackAdam added backport 2.x and removed backport 2.x backport-failed labels Dec 6, 2024

opensearch-trigger-bot bot added the backport-failed label Dec 6, 2024

TackAdam removed backport 2.x backport-failed labels Dec 6, 2024

opensearch-project deleted a comment from opensearch-trigger-bot bot Dec 6, 2024

TackAdam added the backport 2.x label Dec 6, 2024

opensearch-trigger-bot bot added the backport-failed label Dec 6, 2024

TackAdam mentioned this pull request Dec 6, 2024

[Manual Backport 2.x ] 2257 to 2.x Traces - Gantt chart / Span list rework #2283

Merged

6 tasks

TackAdam mentioned this pull request Dec 17, 2024

Gantt Chart / Service Map followup #2294

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Traces - Gantt chart / Span list rework #2257

Traces - Gantt chart / Span list rework #2257

TackAdam commented Nov 18, 2024 •

edited

Loading

ps48 commented Nov 26, 2024

joshuali925 Dec 3, 2024

TackAdam Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 4, 2024

paulstn Dec 4, 2024

TackAdam Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 5, 2024

ps48 Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 4, 2024 •

edited

Loading

ps48 Dec 4, 2024

ps48 Dec 4, 2024

ps48 left a comment

opensearch-trigger-bot bot commented Dec 6, 2024

Traces - Gantt chart / Span list rework #2257

Traces - Gantt chart / Span list rework #2257

Conversation

TackAdam commented Nov 18, 2024 • edited Loading

Demo:

Description

Issues Resolved

Check List

ps48 commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TackAdam Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ps48 left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Dec 6, 2024

TackAdam commented Nov 18, 2024 •

edited

Loading

TackAdam Dec 4, 2024 •

edited

Loading