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.

Sign up for GitHub

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

Jump to bottom

Instances latency distribution chart tooltips and axis fixes #95577

Merged
merged 18 commits into from
Apr 13, 2021
Merged

Instances latency distribution chart tooltips and axis fixes #95577

merged 18 commits into from
Apr 13, 2021

Conversation

smith
Copy link
Contributor

@smith smith commented Mar 26, 2021 •
edited
Loading

Re-enable the instances latency distribution chart on the service overview.

image

When multiple instances are hovered it lists them separately:

image

The x-axis domain is customized to go from 0 to the max of the throughput values, so items don't display right on the origin.

Clicking on an item filters by putting the selected items in the kuery bar.

Create a getServiceNodeName helper that returns "(Empty)" if the value is _service_node_name_missing_.

Fixes #88852
Fixes #92631
Fixes #92870

@smith smith changed the title Instances latency distribution chart tooltips Instances latency distribution chart tooltips and axis fixes Mar 30, 2021
@smith smith added auto-backport Deprecated: Automatically backport this PR after it's merged release_note:enhancement v7.13.0 v8.0.0 labels Mar 30, 2021
@smith smith marked this pull request as ready for review March 30, 2021 18:02
@smith smith requested a review from a team as a code owner March 30, 2021 18:02
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Mar 30, 2021
@elasticmachine
Copy link
Contributor

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

@smith
Copy link
Contributor Author

smith commented Mar 30, 2021

retest

sorenlouv
sorenlouv reviewed Apr 6, 2021
View reviewed changes
...lic/components/shared/charts/instances_latency_distribution_chart/custom_tooltip.stories.tsx
import { PrimaryStatsServiceInstanceItem } from '../../../app/service_overview/service_overview_instances_chart_and_table';
import { CustomTooltip } from './custom_tooltip';

function getLatencyFormatter(props: TooltipInfo) {
Copy link
Member

Choose a reason for hiding this comment

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

We should ask the elastic charts team to make TooltipInfo a generic so we can get better typing of datum:

Suggested change
function getLatencyFormatter(props: TooltipInfo) {
function getLatencyFormatter(props: TooltipInfo<PrimaryStatsServiceInstanceItem>) {

@formgeist
Copy link
Contributor

I'm pretty late to the party here, but the overall tooltip and interactions look good to me as a first pass. I'm designing a more full-fledged interaction design to give a more clear definition of what I imagine should happen within the chart with selection and more.

Would it be possible to add a small hint in the tooltip that the user can click and filter by the selected instance? E.g. "Click to filter by this instance" (someone has better copy?)?

Screenshot 2021-04-06 at 11 29 34

Happy to open this as an enhancement if it doesn't fit in right now.

@sorenlouv
Copy link
Member

sorenlouv commented Apr 6, 2021 •
edited
Loading

Would it be possible to add a small hint in the tooltip that the user can click and filter by the selected instance? E.g. "Click to filter by this instance"

+1.
Additionally, can we trim the service node name that we display?

image

@formgeist
Copy link
Contributor

Would it be possible to add a small hint in the tooltip that the user can click and filter by the selected instance? E.g. "Click to filter by this instance"

+1.
Additionally, can we trim the service node name that we display?

image

Good point! In the Metrics section we truncate the service.node.name after 12 chars. Maybe we can reuse for a consistent truncation pattern?

Screenshot 2021-04-07 at 08 43 49

@formgeist
Copy link
Contributor

Would it be possible to add a small hint in the tooltip that the user can click and filter by the selected instance? E.g. "Click to filter by this instance" (someone has better copy?)?

@smith I imagine something like this in the tooltip

Screenshot 2021-04-08 at 10 59 24

@smith
Copy link
Contributor Author

smith commented Apr 12, 2021

Additionally, can we trim the service node name that we display?

@formgeist in every case where we truncate now, you can see the full text in a tooltip when hovering. In this case we can't show a tooltip and you can't hover that name, so truncating it might hide information that you can't see in its expanded form.

@formgeist
Copy link
Contributor

Additionally, can we trim the service node name that we display?

@formgeist in every case where we truncate now, you can see the full text in a tooltip when hovering. In this case we can't show a tooltip and you can't hover that name, so truncating it might hide information that you can't see in its expanded form.

I'm fine with leaving it as-is for now

@smith
Copy link
Contributor Author

smith commented Apr 13, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1571 1573 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 4.2MB 4.2MB +9.0KB

History

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

@smith smith merged commit dfca5d4 into elastic:master Apr 13, 2021
@smith smith deleted the nls/bubble-tips branch April 13, 2021 21:03
@kibanamachine kibanamachine mentioned this pull request Apr 13, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 13, 2021
@smith @kibanamachine
Instances latency distribution chart tooltips and axis fixes (elastic…
6a2d241 
…#95577)

Fixes elastic#88852
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 13, 2021
@kibanamachine @smith
Instances latency distribution chart tooltips and axis fixes (#95577) (…
defa0f9 
…#97050)

Fixes #88852

Co-authored-by: Nathan L Smith <nathan.smith@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 14, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/support-m…
5b4d940 
…ax_primary_shard_size

* 'master' of github.com:elastic/kibana: (99 commits)
  added missing optional chain for bracket notation (elastic#96939)
  [Discover][DocViewer] Fix toggle columns from doc viewer table tab (elastic#95748)
  [TSVB] Fix per-request caching of index patterns (elastic#97043)
  [Datatable] Fix filter cell flakiness (elastic#96934)
  Unskip heatmap suite and fixes flakiness (elastic#96941)
  [Fleet] Improve performance of data stream API (elastic#97058)
  [ML] Data Frame Analytics: remove beta badge (elastic#96977)
  [App Search] Migrate expanded rows for meta engines table in Engines Overview (elastic#96251)
  Instances latency distribution chart tooltips and axis fixes (elastic#95577)
  [Monitoring] Using primary average shard size (elastic#96177)
  [Workplace Search] Hide Kibana chrome on 3rd party connector redirects (elastic#97028)
  ## [Security Solution] Fixes `Exit full screen` and `Copy to cliboard` styling issues (elastic#96676)
  Index pattern field editor - Add warning on name or type change (elastic#95528)
  [App Search] Add small engine breadcrumb utility helper (elastic#96917)
  Copy esArchiver commands from ./reassign.ts to fix tests (elastic#97012)
  [Security Solution][Detections] Updates MITRE Tactics, Techniques, and Subtechniques for 7.13 (elastic#97011)
  Index patterns server - throw correct error on field caps 404 (elastic#95879)
  Use `EuiThemeProvider` in lists plugin tests and stories (elastic#96129)
  [npm] upgrade caniuse database (elastic#97002)
  chore(NA): moving @kbn/apm-utils into bazel (elastic#96227)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/serialization/policy_serialization.test.ts
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts
phillipb added a commit to phillipb/kibana that referenced this pull request Apr 14, 2021
@phillipb
Merge branch 'master' of github.com:elastic/kibana into add-log-rate-…
79e604e 
…to-metrics-tab

* 'master' of github.com:elastic/kibana: (61 commits)
  [Usage collection] Usage counters (elastic#96696)
  UI actions readme (elastic#96925)
  [TSVB] Enable brush for visualizations created with no index patterns (elastic#96727)
  [Data telemetry] Add Async Search to the tests (elastic#96693)
  added missing optional chain for bracket notation (elastic#96939)
  [Discover][DocViewer] Fix toggle columns from doc viewer table tab (elastic#95748)
  [TSVB] Fix per-request caching of index patterns (elastic#97043)
  [Datatable] Fix filter cell flakiness (elastic#96934)
  Unskip heatmap suite and fixes flakiness (elastic#96941)
  [Fleet] Improve performance of data stream API (elastic#97058)
  [ML] Data Frame Analytics: remove beta badge (elastic#96977)
  [App Search] Migrate expanded rows for meta engines table in Engines Overview (elastic#96251)
  Instances latency distribution chart tooltips and axis fixes (elastic#95577)
  [Monitoring] Using primary average shard size (elastic#96177)
  [Workplace Search] Hide Kibana chrome on 3rd party connector redirects (elastic#97028)
  ## [Security Solution] Fixes `Exit full screen` and `Copy to cliboard` styling issues (elastic#96676)
  Index pattern field editor - Add warning on name or type change (elastic#95528)
  [App Search] Add small engine breadcrumb utility helper (elastic#96917)
  Copy esArchiver commands from ./reassign.ts to fix tests (elastic#97012)
  [Security Solution][Detections] Updates MITRE Tactics, Techniques, and Subtechniques for 7.13 (elastic#97011)
  ...
madirey pushed a commit to madirey/kibana that referenced this pull request May 11, 2021
@smith @madirey
Instances latency distribution chart tooltips and axis fixes (elastic…
285e1ec 
…#95577)

Fixes elastic#88852
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged release_note:enhancement Team:APM All issues that need APM UI Team support v7.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
5 participants
@smith @elasticmachine @formgeist @sorenlouv @kibanamachine