[APM] Service Map Layout #59020

ogupte · 2020-03-02T12:33:12Z

Addresses #55544:

uses the core breadthfirst cytoscape layout
rotates elements by -90degrees
selects rum nodes as roots
implements hover styles to show connected nodes
connects nodes with taxi-style edges
fixes flash of unstyled cytoscape elements on initial load
moves nodes closer to each other

formgeist · 2020-03-02T13:33:22Z

First of all, this looks amazing @ogupte 🎉 Much better-looking layout for the maps. I've got a few comments and questions;

Is it possible that we can decrease the "distance" between each node in the layout? Right now the margin around each node seems big and results in a very zoomed out view at the default level.
The hovered highlights look great!
The placement of the apm-server in your example, would it be possible to place below the client? I remember we discussed that in the last sync, because having it before the client would be a little confusing.

I would love to see this with another dataset to see how it will display the nodes etc.

formgeist

Apart from my comments, I think this is a definite improvement over the existing map layout.

smith

I have some suggestions but nothing blocking. Agree with @formgeist's comments too.

Great work!

smith · 2020-03-02T13:30:53Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/Cytoscape.stories.tsx

+  )
+  .addParameters({
+    options: {
+      // theme: create({


Remove these comments?

smith · 2020-03-02T13:36:08Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/cytoscapeOptions.ts

  }
 ];

 export const cytoscapeOptions: cytoscape.CytoscapeOptions = {
  autoungrabify: true,
  boxSelectionEnabled: false,
-  layout,
+  // layout,


Remove this comment?

smith · 2020-03-02T13:55:42Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/cytoscapeOptions.ts

@@ -75,7 +71,10 @@ const style: cytoscape.Stylesheet[] = [
  {
    selector: 'edge',
    style: {
-      'curve-style': 'bezier',
+      // 'curve-style': 'bezier',


Remove this comment

smith · 2020-03-02T13:59:19Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/Cytoscape.tsx

        cy.removeListener('mouseover', 'edge, node', mouseoverHandler);
        cy.removeListener('mouseout', 'edge, node', mouseoutHandler);
      }
    };
-  }, [cy, serviceName]);
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Why are we disabling exhaustive deps here?

i was able to rearrange the code to safely remove this

smith · 2020-03-02T14:00:18Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/Cytoscape.tsx

+    animationEasing: animationOptions.easing,
+    animationDuration: animationOptions.duration,
+    // Rotate nodes from top -> bottom to display left -> right
+    // @ts-ignore


You can put a description of why we're ignoring something after the @ts-ignore on the same line comment.

@smith great idea.
What about opening a PR to fix the types and link to that in the comment?

@sqren I'll just wait until this is merged and open a PR that updates the types and removes the comments that can be removed.

smith · 2020-03-02T14:02:26Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/Cytoscape.tsx

-  // Set up cytoscape event handlers
-  useEffect(() => {
-    const dataHandler: cytoscape.EventHandler = event => {
+  const dataHandler = useCallback(


The typing of this with the extra argument is weird now, so I think maybe there should be two functions, one that actually is an EventHandler (that you can type with useCallback<cytoscape.EventHandler> and the one that takes the extra argument that gets wrapped.

It's not a big thing, but I'd rather have more verbose code than confusing types.

smith · 2020-03-02T14:03:27Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/cytoscapeOptions.ts

 function isService(el: cytoscape.NodeSingular) {
  return el.data('type') === 'service';
 }

 const style: cytoscape.Stylesheet[] = [
+  // @ts-ignore


If we update to the latest @types/cytoscape some of these will go away. We should. I know visibility is in there as of the latest.

smith · 2020-03-02T14:03:40Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/cytoscapeOptions.ts

@@ -75,7 +71,10 @@ const style: cytoscape.Stylesheet[] = [
  {
    selector: 'edge',
    style: {
-      'curve-style': 'bezier',
+      // 'curve-style': 'bezier',


Remove comment.

smith · 2020-03-02T14:05:13Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/useRefHeight.ts

@@ -8,13 +8,17 @@ import { useWindowSize } from 'react-use';

 export function useRefHeight(): [


This probably should be renamed to something like useRefDimensions since it gets height and width now.

sorenlouv · 2020-03-02T20:30:03Z

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/useRefHeight.ts


  const height = ref.current ? windowHeight - topOffset : 0;

-  return [ref, height];
+  return [ref, height, width];
 }


A quick scan: looks like it would be more clear to return the default value early and the conditionals disappear (untested):

export function useRefHeight(): [ MutableRefObject<HTMLDivElement | null>, number, number ] { const ref = useRef<HTMLDivElement>(null); if (!ref.current) { return [ref, 0, 0]; } const windowHeight = useWindowSize().height; const { top, width } = ref.current.getBoundingClientRect(); const height = windowHeight - top; return [ref, height, width]; }

Btw what do you think about returning an object similar to how useWindowSize works?

return { ref, height, width };

- uses the core breadthfirst cytoscape layout - rotates elements by -90degrees - selects rum nodes as roots - implements hover styles to show connected nodes - fixes flash of unstyled cytoscape elements on initial load

kibanamachine · 2020-03-03T20:56:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 83715c7

History

💔 Build #30283 failed e8d955e
💔 Build #30026 failed 56153dab69df52301718180fbe38437c3ec43928

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Addresses elastic#55544. - uses the core breadthfirst cytoscape layout - rotates elements by -90degrees - selects rum nodes as roots - implements hover styles to show connected nodes - fixes flash of unstyled cytoscape elements on initial load * PR review feedback * adds canned response for testing cytoscape layout in storybook * update dep snapshot for removing cytoscape-dagre

* Addresses #55544. - uses the core breadthfirst cytoscape layout - rotates elements by -90degrees - selects rum nodes as roots - implements hover styles to show connected nodes - fixes flash of unstyled cytoscape elements on initial load * PR review feedback * adds canned response for testing cytoscape layout in storybook * update dep snapshot for removing cytoscape-dagre

ogupte added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 2, 2020

ogupte requested review from smith and formgeist March 2, 2020 12:33

ogupte requested a review from a team as a code owner March 2, 2020 12:33

formgeist approved these changes Mar 2, 2020

View reviewed changes

smith approved these changes Mar 2, 2020

View reviewed changes

sorenlouv reviewed Mar 2, 2020

View reviewed changes

ogupte added 3 commits March 2, 2020 23:37

Addresses elastic#55544.

d3c851e

- uses the core breadthfirst cytoscape layout - rotates elements by -90degrees - selects rum nodes as roots - implements hover styles to show connected nodes - fixes flash of unstyled cytoscape elements on initial load

PR review feedback

f91744f

adds canned response for testing cytoscape layout in storybook

e8d955e

ogupte force-pushed the apm-55544-service-map-layout branch from 56153da to e8d955e Compare March 3, 2020 07:48

update dep snapshot for removing cytoscape-dagre

83715c7

sorenlouv approved these changes Mar 4, 2020

View reviewed changes

ogupte merged commit 5539d69 into elastic:master Mar 4, 2020

ogupte mentioned this pull request Mar 4, 2020

[7.x] [APM] Service Map Layout (#59020) #59261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Service Map Layout #59020

[APM] Service Map Layout #59020

ogupte commented Mar 2, 2020 •

edited

Loading

formgeist commented Mar 2, 2020

formgeist left a comment

smith left a comment

smith Mar 2, 2020

smith Mar 2, 2020

smith Mar 2, 2020

smith Mar 2, 2020

ogupte Mar 3, 2020

smith Mar 2, 2020

sorenlouv Mar 2, 2020

smith Mar 3, 2020

smith Mar 2, 2020

smith Mar 2, 2020

smith Mar 2, 2020

smith Mar 2, 2020

sorenlouv Mar 2, 2020 •

edited

Loading

kibanamachine commented Mar 3, 2020

		@@ -8,13 +8,17 @@ import { useWindowSize } from 'react-use';

		export function useRefHeight(): [

[APM] Service Map Layout #59020

[APM] Service Map Layout #59020

Conversation

ogupte commented Mar 2, 2020 • edited Loading

formgeist commented Mar 2, 2020

formgeist left a comment

Choose a reason for hiding this comment

smith left a 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

sorenlouv Mar 2, 2020 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Mar 3, 2020

💚 Build Succeeded

History

ogupte commented Mar 2, 2020 •

edited

Loading

sorenlouv Mar 2, 2020 •

edited

Loading