-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[APM] Service maps layout enhancements #76481
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
Conversation
…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.
|
Pinging @elastic/apm-ui (Team:apm) |
| cy.on('select', 'node', selectHandler); | ||
| cy.on('unselect', 'node', deselect); | ||
| cy.on('data viewport', deselect); | ||
| cy.on('drag', 'node', deselect); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| }, | ||
| ]; | ||
| return <Cytoscape elements={elements} height={600} width={1340} />; | ||
| return <Cytoscape elements={elements} height={600} />; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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θ, | ||
| }; | ||
| } |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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]); |
There was a problem hiding this comment.
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?
sorenlouv
left a comment
There was a problem hiding this 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! 👍
|
@ogupte can you add an screen capture where you demo the dragging functionality? |
|
The dragging vs. click actually works quite flawlessly. Looking really good |
…e extension - Adds attribution for `applyCubicBezierStyles`
spalger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, LGTM!
| */ | ||
| export async function generateNoticeFromSource({ productName, directory, log }: Options) { | ||
| const globs = ['**/*.{js,less,css,ts}']; | ||
| const globs = ['**/*.{js,less,css,ts,tsx}']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😨
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
distributable file count
History
To update your PR or re-run it, just comment with: |
* 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
* 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
…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
…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) ...
|
Bugs found:
Tests ok on all browsers besides the bugs. |
Closes #71770 for APM service maps by replacing
breadthfirstlayout with one from thecytoscape-dagreextension. Also replaces thetaxiedges with approximated cubicunbundled-bezieredges. 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.Drag and drop nodes:
