Skip to content
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

[APM] Service maps layout enhancements #76481

Merged

Conversation

ogupte
Copy link
Contributor

@ogupte ogupte commented Sep 2, 2020

Closes #71770 for APM service maps by replacing breadthfirst layout with one from the cytoscape-dagre extension. Also replaces the taxi edges with approximated cubic unbundled-bezier edges. Finally, this adds the ability to drag individual nodes around the service map. Also fixes storybook anomaly score generation and better utilizes available vertical space in storybook.

Screen Shot 2020-09-02 at 1 48 47 AM

Drag and drop nodes:
service-maps-layout-drag

…ayout with

one from the cytoscape-dagre extension. Also replaces the taxi edges
with cubic bezier edges. Finally, this adds the ability to drag
individual nodes around the service map.
@ogupte ogupte requested a review from a team as a code owner September 2, 2020 09:05
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Sep 2, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@@ -71,13 +71,15 @@ export function Popover({ focusedServiceName }: PopoverProps) {
cy.on('select', 'node', selectHandler);
cy.on('unselect', 'node', deselect);
cy.on('data viewport', deselect);
cy.on('drag', 'node', deselect);
Copy link
Member

Choose a reason for hiding this comment

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

Will dragging cause the popover to show for a brief period of time? Or will it not be displayed at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Node dragging doesn't trigger the select event, so it won't display the popover at all.

@@ -330,7 +328,7 @@ storiesOf('app/ServiceMap/Cytoscape', module)
},
},
];
return <Cytoscape elements={elements} height={600} width={1340} />;
return <Cytoscape elements={elements} height={600} />;
Copy link
Member

Choose a reason for hiding this comment

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

Why did we hardcode the width in the first place? And why is it no longer needed? (same question about height)

Copy link
Member

Choose a reason for hiding this comment

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

oh, this is for storybook. Then it's not a big deal 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did we hardcode the width in the first place? And why is it no longer needed? (same question about height)

The width was needed to define the boundingBox of the previous layout. This option isn't necessary with the new layout, so i was able to remove it. height is still necessary to set the root div height (which is always full width).

x: x * cosθ - y * sinθ,
y: x * sinθ + y * cosθ,
};
}
Copy link
Member

Choose a reason for hiding this comment

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

No more custom calculations? Feels good!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the new layout supports left->right rank direction out of the box!

-alpha * y * costheta,
alpha * y * costheta,
]);
edge.style('control-point-weights', [alpha, 1 - alpha]);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment to this section what this is doing?

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Super excited about this! 👍

@sorenlouv
Copy link
Member

@ogupte can you add an screen capture where you demo the dragging functionality?

@formgeist
Copy link
Contributor

The dragging vs. click actually works quite flawlessly. Looking really good

…e extension

- Adds attribution for `applyCubicBezierStyles`
@ogupte ogupte requested a review from a team as a code owner September 2, 2020 21:38
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Good catch, LGTM!

@@ -41,7 +41,7 @@ interface Options {
* into the repository.
*/
export async function generateNoticeFromSource({ productName, directory, log }: Options) {
const globs = ['**/*.{js,less,css,ts}'];
const globs = ['**/*.{js,less,css,ts,tsx}'];
Copy link
Contributor

Choose a reason for hiding this comment

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

😨

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
apm 1236 +289 947

async chunks size

id value diff baseline
apm 4.9MB +160.9KB 4.7MB

distributable file count

id value diff baseline
total 45608 +77 45531

History

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

@ogupte ogupte merged commit d3416c0 into elastic:master Sep 3, 2020
ogupte added a commit to ogupte/kibana that referenced this pull request Sep 3, 2020
* Fixes storybook anomaly score generation and better utilizes available screen space

* Closes elastic#71770 for APM service maps by replacing breadthfirst layout with
one from the cytoscape-dagre extension. Also replaces the taxi edges
with cubic bezier edges. Finally, this adds the ability to drag
individual nodes around the service map.

* Removes unused code

* removes commented line of code

* - Adds ability for scripts/notice.js to check files with the .tsx file extension
- Adds attribution for `applyCubicBezierStyles`

* Refine comment text and MIT license url
ogupte added a commit that referenced this pull request Sep 3, 2020
* Fixes storybook anomaly score generation and better utilizes available screen space

* Closes #71770 for APM service maps by replacing breadthfirst layout with
one from the cytoscape-dagre extension. Also replaces the taxi edges
with cubic bezier edges. Finally, this adds the ability to drag
individual nodes around the service map.

* Removes unused code

* removes commented line of code

* - Adds ability for scripts/notice.js to check files with the .tsx file extension
- Adds attribution for `applyCubicBezierStyles`

* Refine comment text and MIT license url
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 3, 2020
…nes/processors-forms-k-s

* 'master' of github.com:elastic/kibana: (216 commits)
  [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558)
  [Security Solution] add an excess validation instead of the exact match (elastic#76472)
  Introduce TS incremental builds & move src/test_utils to TS project  (elastic#76082)
  fix bad merge (elastic#76629)
  [Newsfeed] Ensure the version format when calling the API (elastic#76381)
  remove server_extensions mixin (elastic#76606)
  Remove legacy applications and legacy mode (elastic#75987)
  [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749)
  Replace FetchOptions with ISearchOptions (elastic#76538)
  Move streams utils to the core  (elastic#76397)
  [Ingest Manager] Wording & linking improvements (elastic#76284)
  remove legacy kibana plugin (elastic#76064)
  [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379)
  Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482)
  [APM] Service maps layout enhancements (elastic#76481)
  [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586)
  [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544)
  Index Pattern class - remove toJSON and toString (elastic#76246)
  skip failing suite (elastic#76581)
  Split up and clarify Enterprise Search codeowners (elastic#76580)
  ...

# Conflicts:
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processor_settings_fields.tsx
#	x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/foreach.tsx
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 3, 2020
…oleysens/kibana into ingest-pipelines/processors-forms-k-s

* 'ingest-pipelines/processors-forms-k-s' of github.com:jloleysens/kibana: (216 commits)
  [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558)
  [Security Solution] add an excess validation instead of the exact match (elastic#76472)
  Introduce TS incremental builds & move src/test_utils to TS project  (elastic#76082)
  fix bad merge (elastic#76629)
  [Newsfeed] Ensure the version format when calling the API (elastic#76381)
  remove server_extensions mixin (elastic#76606)
  Remove legacy applications and legacy mode (elastic#75987)
  [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749)
  Replace FetchOptions with ISearchOptions (elastic#76538)
  Move streams utils to the core  (elastic#76397)
  [Ingest Manager] Wording & linking improvements (elastic#76284)
  remove legacy kibana plugin (elastic#76064)
  [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379)
  Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482)
  [APM] Service maps layout enhancements (elastic#76481)
  [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586)
  [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544)
  Index Pattern class - remove toJSON and toString (elastic#76246)
  skip failing suite (elastic#76581)
  Split up and clarify Enterprise Search codeowners (elastic#76580)
  ...
@cauemarcondes cauemarcondes self-assigned this Oct 13, 2020
@cauemarcondes
Copy link
Contributor

@cauemarcondes cauemarcondes added the apm:test-plan-done Pull request that was successfully tested during the test plan label Oct 14, 2020
@bmorelli25 bmorelli25 mentioned this pull request Oct 15, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:test-plan-done Pull request that was successfully tested during the test plan release_note:enhancement Team:APM All issues that need APM UI Team support v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Service map: layout improvements (timeboxed exploration)
7 participants