Skip to content

[APM] Service Map - Separate overlapping edges by rotating nodes#60477

Merged
ogupte merged 3 commits intoelastic:masterfrom
ogupte:apm-59779-rotate-service-map-separate-edges
Mar 21, 2020
Merged

[APM] Service Map - Separate overlapping edges by rotating nodes#60477
ogupte merged 3 commits intoelastic:masterfrom
ogupte:apm-59779-rotate-service-map-separate-edges

Conversation

@ogupte
Copy link
Contributor

@ogupte ogupte commented Mar 18, 2020 •
edited
Loading

Closes #59779. Adds rotation transform which does the top->bottom to left->right transformation + an extra 5 degrees which results in taxi edges separating when rendered.
service-maps-layout-8

@ogupte ogupte added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 18, 2020
@ogupte ogupte requested a review from a team March 18, 2020 07:13
@ogupte ogupte self-assigned this Mar 18, 2020
@formgeist
Copy link
Contributor

formgeist commented Mar 18, 2020

I like this a lot, it really helps to see the connections ongoing and outgoing. I wonder if the thickness of the highlighted taxi lines are too heavy, because it almost overlays all the others when you hover a single node. If the default stroke thickness is 1, can we perhaps have the highlighted thickness just 2? We will color it with the blue primary in your other PR, so I think it'll be clear enough with these two stylings.

smith
smith approved these changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

I like it. Code is approved, but have a look at @formgeist comment on the edge looks.

x-pack/legacy/plugins/apm/public/components/app/ServiceMap/Cytoscape.tsx Outdated
Copy link
Contributor

@smith smith Mar 18, 2020

Choose a reason for hiding this comment

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

+1,000,000,000 points for using θs in variable names.

@ogupte
Copy link
Contributor Author

ogupte commented Mar 18, 2020 •
edited
Loading

I like this a lot, it really helps to see the connections ongoing and outgoing. I wonder if the thickness of the highlighted taxi lines are too heavy, because it almost overlays all the others when you hover a single node. If the default stroke thickness is 1, can we perhaps have the highlighted thickness just 2? We will color it with the blue primary in your other PR, so I think it'll be clear enough with these two stylings.

@formgeist Should there be any color/shade indication of hover as well as the thickness change?

@formgeist
Copy link
Contributor

formgeist commented Mar 18, 2020

@formgeist Should there be any color/shade indication of hover as well as the thickness change?

I think you already implemented the different color on node hover, but I wanted to go something along the lines of this;

Screenshot 2020-03-18 at 16 33 49

@ogupte ogupte force-pushed the apm-59779-rotate-service-map-separate-edges branch from d36b7ac to ac7c926 Compare March 20, 2020 08:06
@formgeist
Copy link
Contributor

formgeist commented Mar 20, 2020

The visual changes look good to me - I think the darker shade on edges on hover looks really good, much clearer than before.

@formgeist
Copy link
Contributor

formgeist commented Mar 20, 2020

@ogupte Could you include a minor copy update in this PR? I realized that we don't pluralize Service map, so I would like to change the placeholder copy in the Kuerybar.

Screenshot 2020-03-20 at 10 32 43

Change to: Search is not available for service map.

Thanks!

ogupte added 3 commits March 20, 2020 12:46
@ogupte
Adds rotation transform which does the top->bottom to left->right
96c7665 
transformation + an extra 5 degrees which results in taxi edges
separating when rendered.
@ogupte
PR feedback to reduce edge width on hover, and assure that connected
789c0ca 
edges are highlighted when node is selected/focused
@ogupte
update disabled kuery bar placeholder text for service map
3fbefbd
@ogupte ogupte force-pushed the apm-59779-rotate-service-map-separate-edges branch from ac7c926 to 3fbefbd Compare March 20, 2020 19:46
@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 20, 2020

💚 Build Succeeded

History

  • 💚 Build #34797 succeeded ac7c92631f8639ccf49b72e34e1dc0fe4fdd6b73
  • 💚 Build #34089 succeeded d36b7ac34a61fc5a9a3331671ab9a15937cb891f

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

@ogupte ogupte merged commit 9de2d81 into elastic:master Mar 21, 2020
ogupte added a commit to ogupte/kibana that referenced this pull request Mar 21, 2020
@ogupte
[APM] Service Map - Separate overlapping edges by rotating nodes (ela…
98aa402 
…stic#60477)

* Adds rotation transform which does the top->bottom to left->right
transformation + an extra 5 degrees which results in taxi edges
separating when rendered.

* PR feedback to reduce edge width on hover, and assure that connected
edges are highlighted when node is selected/focused

* update disabled kuery bar placeholder text for service map
@ogupte ogupte mentioned this pull request Mar 21, 2020
Merged
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 22, 2020
@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 22, 2020

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 23, 2020

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 23, 2020
@gmmorris
Merge branch 'master' into alerting/tls-warning
6230180 
* master: (39 commits)
  [APM]Create custom link from Trace summary (elastic#59648)
  [ML] Fixing app clean up (elastic#60853)
  [SIEM] Use ECS categorisation for Authentication widgets (elastic#60734)
  [NP] Remove kbnUrl usage in discover/dashboard/visualize (elastic#60016)
  Skip failing test
  [Uptime]Update fetch effect failed action handling (elastic#60742)
  [npm] upgrade elastic/maki (elastic#60829)
  [Uptime] Add Settings Page (elastic#53550)
  [APM] service maps: avoid unnecesary `useDeepObjectIdentity` (elastic#60836)
  [Index management] Re-enable index template tests (elastic#60780)
  Fixed UI/UX issues: alerts delete confirmation, combobox behaviors (elastic#60703)
  [SIEM] Fix patching of ML Rules (elastic#60830)
  [APM] Service Map - Separate overlapping edges by rotating nodes (elastic#60477)
  [Alerting] fix flaky test for index threshold grouping (elastic#60792)
  [SIEM][Detection Engine] Adds test scripts for machine learning feature
  Flatten child api response for resolver (elastic#60810)
  Change "url" to "urls" in APM agent instructions (elastic#60790)
  [DOCS] Updates API requests and examples (elastic#60695)
  [SIEM] [Cases] Create case from timeline (elastic#60711)
  [Lens] Resetting a layer generates new suggestions (elastic#60674)
  ...
ogupte added a commit that referenced this pull request Mar 23, 2020
@ogupte @elasticmachine
[APM] Service Map - Separate overlapping edges by rotating nodes (#60477
49f4a1f 
) (#60843)

* Adds rotation transform which does the top->bottom to left->right
transformation + an extra 5 degrees which results in taxi edges
separating when rendered.

* PR feedback to reduce edge width on hover, and assure that connected
edges are highlighted when node is selected/focused

* update disabled kuery bar placeholder text for service map

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@smith smith smith approved these changes

Assignees

@ogupte ogupte

Labels

release_note:skip Skip the PR/issue when compiling release notes v7.7.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[APM] Serivce map fix overlapping connection edges

4 participants

@ogupte @formgeist @kibanamachine @smith