[Services map] Update redirection/Focus field rework #2264

TackAdam · 2024-11-27T18:56:52Z

Demo:

ServiceMap.mp4

Description

Updated the focus field to no longer apply a filter, it now just renders the connected services and allows the user to investigate using the focus window. The drop down now reflects the connected viewable services to switch to. The reset icon now clears the existing filter if it matches the focus field and is currently applied. The focus field is now disabled while a filter of an existing service is active.

Screen.Recording.2024-12-02.at.9.46.22.AM.mov

Clicking a service currently causes the title clicked to be set as a filter.

Screen.Recording.2024-11-27.at.10.53.01.AM.mov

Updated behavior now re-directs the user to the corresponding service view page

Screen.Recording.2024-11-27.at.10.54.14.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.

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

public/components/trace_analytics/components/services/services_table.tsx

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

ps48 · 2024-12-02T23:49:05Z

public/components/trace_analytics/components/services/service_view.tsx

+  let serviceId: string | null = null;
+
+  try {
+    const location = useLocation();
+    const params = new URLSearchParams(location?.search || '');
+    serviceId = params.get('serviceId');
+  } catch (error) {
+    serviceId = null;
+  }


Can we use hooks and state here to make it cleaner?

const location = useLocation(); useEffect(() => { try { const params = new URLSearchParams(location.search); const id = params.get('serviceId'); setServiceId(id); } catch (error) { setServiceId(null); } }, [location]);

Location is causing the service view page to not render as it is not defined, so it needs to be within the "Try".
And the useLocation component cannot be called inside a React Hook.

Josh helped find the issue

dashboards-observability/public/components/trace_analytics/home.tsx

Lines 493 to 494 in a1dfaea

</HashRouter>

{flyout}

Fly-out was not inside the HashRouter.

public/components/trace_analytics/components/services/services_table.tsx

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

joshuali925 · 2024-12-03T23:18:18Z

public/components/trace_analytics/components/common/plots/service_map.tsx

+  filters: FilterType[];
+  setFilters:
+    | React.Dispatch<React.SetStateAction<FilterType[]>>
+    | ((filters: FilterType[]) => void);


curious why are both needed?

Thought i needed to cover other conditions, removed it and seems to work fine.
Thanks

joshuali925 · 2024-12-03T23:22:44Z

public/components/trace_analytics/components/common/plots/service_map.tsx

+          ticks,
+          service,
+          serviceMap[service]?.relatedServices,
+          true // Enable filtering to focus on connected nodes


i didn't look into getServiceMapGraph, but seems service and serviceMap[service]?.relatedServices are used for filtering, and they will be absent if we don't want filtering. Then is this boolean required?

The focus functionality is not applying a filter so for the new experience these are required to change the options that show up using the focus functionality.

Without:

Screen.Recording.2024-12-04.at.11.13.22.AM.mov

With:

Screen.Recording.2024-12-04.at.11.14.01.AM.mov

joshuali925 · 2024-12-03T23:23:48Z

public/components/trace_analytics/components/common/plots/service_map.tsx

        serviceMap[currService!]?.relatedServices,
-        filterByCurrService
+        showRelatedServices
      )
    );
  }, [serviceMap, idSelected]);


do we need focusedService and filterByCurrService in dependency array?

I don't believe so

Screen.Recording.2024-12-04.at.11.17.18.AM.mov

We should include every dependency in the dependency array even if it seems to pointlessly re-render as to avoid 'stale' states that might lead to bugs that would be difficult to fix.
React useEffect doc: https://react.dev/reference/react/useEffect

joshuali925 · 2024-12-03T23:25:59Z

public/components/trace_analytics/components/services/service_view.tsx

+  let serviceId: string | null = null;
+
+  try {
+    const location = useLocation();


Location is causing the service view page to not render as it is not defined, so it needs to be within the "Try".

do you have more details on what the error is? is it happening when calling useLocation?

Resolved with moving the flyout component into router.

joshuali925 · 2024-12-03T23:26:05Z

public/components/trace_analytics/components/services/service_view.tsx

@@ -76,6 +77,18 @@ export function ServiceView(props: ServiceViewProps) {
  const [redirect, setRedirect] = useState(false);
  const [actionsMenuPopover, setActionsMenuPopover] = useState(false);

+  let serviceId: string | null = null;


should this be a state?

Changed to a state.

ps48 · 2024-12-04T20:11:52Z

public/components/trace_analytics/components/services/__tests__/service_view.test.tsx

+jest.mock('../../../../../../test/__mocks__/coreMocks', () => ({
+  coreStartMock: {
+    chrome: { setBreadcrumbs: jest.fn() },
+    http: { post: jest.fn() },


Can we please add mocks with some actual data (both empty and non-empty response)?

Cypress added for service map
Tests focus functionality in Service map
Testing the re-renders and filtering

Added
import toJson from 'enzyme-to-json';
To service map test

public/components/trace_analytics/components/common/plots/service_map.tsx

public/components/trace_analytics/components/common/plots/__tests__/service_map.test.tsx

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

TackAdam · 2024-12-06T19:27:44Z

Updated applications to no longer filter on name click. Instead it opens the fly-out for details. Also removed the actions column when in the applications page.

Screen.Recording.2024-12-06.at.11.25.15.AM.mov

ps48 · 2024-12-06T21:25:38Z

Updated applications to no longer filter on name click. Instead it opens the fly-out for details. Also removed the actions column when in the applications page.
Screen.Recording.2024-12-06.at.11.25.15.AM.mov

Does the Applications trace navigation work as expected?

TackAdam · 2024-12-06T21:32:13Z

Updated applications to no longer filter on name click. Instead it opens the fly-out for details. Also removed the actions column when in the applications page.
Screen.Recording.2024-12-06.at.11.25.15.AM.mov

Does the Applications trace navigation work as expected?

Clicking Trace applies the filter and switches to the Traces and Spans tab.

Screen.Recording.2024-12-06.at.1.30.04.PM.mov

ps48 · 2024-12-06T21:45:06Z

public/components/trace_analytics/components/common/plots/__tests__/service_map.test.tsx

    const wrapper = mount(<ServiceMap {...defaultProps} />);
-    expect(wrapper).toMatchSnapshot();
+    wrapper.update();
+


Should we wait for the canvas to appear, so that map renders in the snapshot?

ps48 · 2024-12-06T22:27:36Z

Also, looks like this test 1) "before each" hook for "Verifies tree view expand arrow functionality" is flaky we should solve this preferably now or in a follow up PR.

Signed-off-by: Shenoy Pratik <[email protected]>

ps48 · 2024-12-07T18:25:03Z

I see one failure in test because the nodes changes position. We do have a set seed which makes sure the graph has nodes on the position. Need to check what's causing the change cc: @TackAdam

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

TackAdam · 2024-12-07T23:40:37Z

I see one failure in test because the nodes changes position. We do have a set seed which makes sure the graph has nodes on the position. Need to check what's causing the change cc: @TackAdam

The position is consistent but it seems cypress environment used here might be causing the physics to behave slightly different than we see testing locally on original layout. As the point of this test is to ensure the canvas is rendering the nodes my solution is to first focus on a node to cause less to be rendered and ensure that it matches the layout we see locally. This still ensures the nodes are displayed and are intractable. Ran multiple times and seems to be working with my fix.

.cypress/integration/trace_analytics_test/trace_analytics_services.spec.js

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

* Change service table click to page redirection Signed-off-by: Adam Tackett <[email protected]> * remove focus on filtering, bug fixes Signed-off-by: Adam Tackett <[email protected]> * add jest test for url redirection on click Signed-off-by: Adam Tackett <[email protected]> * remove filter on focus, update focus functionality, add testing Signed-off-by: Adam Tackett <[email protected]> * add query filtering back Signed-off-by: Adam Tackett <[email protected]> * change url check to use hook state Signed-off-by: Adam Tackett <[email protected]> * fix location error Signed-off-by: Adam Tackett <[email protected]> * move flyout into hashrouter, remove extra filter Signed-off-by: Adam Tackett <[email protected]> * fix applications, address comments Signed-off-by: Adam Tackett <[email protected]> * fix flaky application test Signed-off-by: Adam Tackett <[email protected]> * make service map snapshop deep Signed-off-by: Adam Tackett <[email protected]> * add service map focus cypress testing Signed-off-by: Adam Tackett <[email protected]> * add more tests to cover service map load and click Signed-off-by: Shenoy Pratik <[email protected]> * add comments to cypress Signed-off-by: Shenoy Pratik <[email protected]> * remove only from cypress tests Signed-off-by: Shenoy Pratik <[email protected]> * update snapshots Signed-off-by: Shenoy Pratik <[email protected]> * adjust cypress test Signed-off-by: Adam Tackett <[email protected]> * adjust cypress testw Signed-off-by: Adam Tackett <[email protected]> * remove only and comment Signed-off-by: Adam Tackett <[email protected]> * switch test back Signed-off-by: Adam Tackett <[email protected]> * add dependancy to use effect Signed-off-by: Adam Tackett <[email protected]> --------- Signed-off-by: Adam Tackett <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Co-authored-by: Adam Tackett <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]> (cherry picked from commit c22c69d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Change service table click to page redirection * remove focus on filtering, bug fixes * add jest test for url redirection on click * remove filter on focus, update focus functionality, add testing * add query filtering back * change url check to use hook state * fix location error * move flyout into hashrouter, remove extra filter * fix applications, address comments * fix flaky application test * make service map snapshop deep * add service map focus cypress testing * add more tests to cover service map load and click * add comments to cypress * remove only from cypress tests * update snapshots * adjust cypress test * adjust cypress testw * remove only and comment * switch test back * add dependancy to use effect --------- (cherry picked from commit c22c69d) Signed-off-by: Adam Tackett <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Adam Tackett <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]>

Change service table click to page redirection

6396b33

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

TackAdam added backport 2.x enhancement New feature or request labels Nov 27, 2024

ps48 reviewed Nov 27, 2024

View reviewed changes

public/components/trace_analytics/components/services/services_table.tsx Outdated Show resolved Hide resolved

Adam Tackett and others added 5 commits November 27, 2024 16:06

remove focus on filtering, bug fixes

87cdd23

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

Merge branch 'main' into serviceViewBug

c828db3

add jest test for url redirection on click

45af580

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

remove filter on focus, update focus functionality, add testing

9147f5b

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

add query filtering back

f50422e

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

TackAdam added the mds-2.18.0 label Dec 2, 2024

TackAdam marked this pull request as ready for review December 2, 2024 17:48

TackAdam changed the title ~~Services update redirection~~ [Services map] Update redirection/Focus field rework Dec 2, 2024

ps48 reviewed Dec 2, 2024

View reviewed changes

public/components/trace_analytics/components/services/services_table.tsx Outdated Show resolved Hide resolved

Adam Tackett added 2 commits December 2, 2024 16:37

change url check to use hook state

10ef093

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

fix location error

8bc5523

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

joshuali925 reviewed Dec 3, 2024

View reviewed changes

ps48 reviewed Dec 4, 2024

View reviewed changes

public/components/trace_analytics/components/common/plots/service_map.tsx Outdated Show resolved Hide resolved

ps48 reviewed Dec 4, 2024

View reviewed changes

public/components/trace_analytics/components/common/plots/__tests__/service_map.test.tsx Outdated Show resolved Hide resolved

Adam Tackett and others added 10 commits December 4, 2024 13:13

move flyout into hashrouter, remove extra filter

c1ac1c2

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

Merge branch 'main' into serviceViewBug

35774e2

Merge branch 'main' into serviceViewBug

c5408a1

Merge branch 'main' into serviceViewBug

69a7d44

fix applications, address comments

1877540

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

Merge branch 'main' into serviceViewBug

03610bb

fix flaky application test

04e30f9

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

make service map snapshop deep

b5e4351

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

add service map focus cypress testing

4781348

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

Merge branch 'main' into serviceViewBug

7a14d1f

Merge branch 'main' into serviceViewBug

2645c66

ps48 reviewed Dec 6, 2024

View reviewed changes

TackAdam and others added 5 commits December 6, 2024 15:09

Merge branch 'main' into serviceViewBug

977d8d1

add more tests to cover service map load and click

9ffec79

Signed-off-by: Shenoy Pratik <[email protected]>

add comments to cypress

5faeeb6

Signed-off-by: Shenoy Pratik <[email protected]>

remove only from cypress tests

747b1b6

Signed-off-by: Shenoy Pratik <[email protected]>

update snapshots

8b2af4f

Signed-off-by: Shenoy Pratik <[email protected]>

Adam Tackett added 2 commits December 7, 2024 12:33

adjust cypress test

b2a2f1c

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

adjust cypress testw

77bc6f4

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

ps48 reviewed Dec 7, 2024

View reviewed changes

.cypress/integration/trace_analytics_test/trace_analytics_services.spec.js Outdated Show resolved Hide resolved

ps48 reviewed Dec 7, 2024

View reviewed changes

.cypress/integration/trace_analytics_test/trace_analytics_services.spec.js Outdated Show resolved Hide resolved

ps48 reviewed Dec 8, 2024

View reviewed changes

.cypress/integration/trace_analytics_test/trace_analytics_services.spec.js Outdated Show resolved Hide resolved

Adam Tackett added 2 commits December 7, 2024 16:20

remove only and comment

3bb65ec

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

switch test back

87017b1

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

ps48 approved these changes Dec 9, 2024

View reviewed changes

Merge branch 'main' into serviceViewBug

30bcd25

joshuali925 approved these changes Dec 10, 2024

View reviewed changes

paulstn approved these changes Dec 10, 2024

View reviewed changes

add dependancy to use effect

c430f5c

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

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

opensearch-trigger-bot bot mentioned this pull request Dec 10, 2024

[Backport 2.x] [Services map] Update redirection/Focus field rework #2292

Merged

TackAdam mentioned this pull request Dec 17, 2024

Gantt Chart / Service Map followup #2294

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Services map] Update redirection/Focus field rework #2264

[Services map] Update redirection/Focus field rework #2264

TackAdam commented Nov 27, 2024 •

edited

Loading

ps48 Dec 2, 2024

TackAdam Dec 3, 2024 •

edited

Loading

TackAdam Dec 4, 2024

joshuali925 Dec 3, 2024

TackAdam Dec 4, 2024

joshuali925 Dec 3, 2024

TackAdam Dec 4, 2024

joshuali925 Dec 3, 2024

TackAdam Dec 4, 2024

paulstn Dec 9, 2024

joshuali925 Dec 3, 2024

TackAdam Dec 4, 2024

joshuali925 Dec 3, 2024

ps48 Dec 4, 2024 •

edited

Loading

TackAdam Dec 4, 2024

ps48 Dec 4, 2024

TackAdam Dec 6, 2024

TackAdam commented Dec 6, 2024

ps48 commented Dec 6, 2024

TackAdam commented Dec 6, 2024

ps48 Dec 6, 2024

ps48 commented Dec 6, 2024 •

edited

Loading

ps48 commented Dec 7, 2024

TackAdam commented Dec 7, 2024

[Services map] Update redirection/Focus field rework #2264

[Services map] Update redirection/Focus field rework #2264

Conversation

TackAdam commented Nov 27, 2024 • edited Loading

Demo:

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

TackAdam Dec 3, 2024 • edited Loading

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

ps48 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

Choose a reason for hiding this comment

TackAdam commented Dec 6, 2024

ps48 commented Dec 6, 2024

TackAdam commented Dec 6, 2024

Choose a reason for hiding this comment

ps48 commented Dec 6, 2024 • edited Loading

ps48 commented Dec 7, 2024

TackAdam commented Dec 7, 2024

TackAdam commented Nov 27, 2024 •

edited

Loading

TackAdam Dec 3, 2024 •

edited

Loading

ps48 Dec 4, 2024 •

edited

Loading

ps48 commented Dec 6, 2024 •

edited

Loading