Adds event loop delay percentiles to mean delay #121052

TinaHeiligers · 2021-12-12T18:38:11Z

Resolves #120667

Kibana's status page shows a summary of Kibana's process metrics and plugin statuses. At the moment, the page displays if there's a problem (or someone navigates directly to /status) meaning that users rely on the information available to help them sort out issues.

This PR adds the event loop delay metrics, showing a subset of the percentiles as detail to the tile that displays the mean.
This PR also enhances the Load and Response time metric tiles, thereby guiding users to what the metrics indicate.

Internal only: Click here to see the PR on cloud

Before:

After:

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately No changes to public API's.

Release Note:

Displays event loop delay metrics in the Status page to assist with performance monitoring.

TinaHeiligers · 2021-12-12T18:42:23Z

src/core/public/core_app/status/components/metric_tiles.tsx

-      layout="horizontal"
-      title={formatMetric(metric)}
-      description={name}
+    <EuiStat


I ended up using a combination of the suggestions from design here, using EuiStat as a footer to the existing EuiCard in MetricTile. EuiCard with horizontal layout doesn't support a footer so I had to go with aligning the text instead.

TinaHeiligers · 2021-12-12T19:00:11Z

src/core/public/core_app/status/components/metric_tiles.tsx

  const metrics = Array.isArray(value) ? value : [value];
  return metrics.map((metric) => formatNumber(metric, type)).join(', ');
 };

 const formatMetricId = ({ name }: Metric) => {
  return name.toLowerCase().replace(/[ ]+/g, '-');
 };
+
+const formatDelayFooterTitle = (meta: MetricMeta) => {


I extracted the formatting to make it easier to read. We could do something similar with the others and maybe even move them into a single formatDetails.ts file if we need to.

TinaHeiligers · 2021-12-12T19:05:19Z

src/core/public/core_app/status/components/metric_tiles.tsx

+        }
+      />
+    );
+  } else if (name === 'Response time avg') {


I grouped the Response time metrics together for two reasons:

The average makes more sense when one knows what it's relative to (the max). Having the min in here would be nice down the line too.

Allows us to use 2 lines of 3 cards rather than 3 lines where the last one only has 2 cards. We could change the flex on these components but this is more toward Design's recommendations or grouping relevant metrics together.

TinaHeiligers · 2021-12-12T19:09:40Z

src/core/public/core_app/status/lib/load_status.ts

-      name: i18n.translate('core.statusPage.metricsTiles.columns.resTimeAvgHeader', {
-        defaultMessage: 'Response time avg',
+      name: i18n.translate('core.statusPage.metricsTiles.columns.processDelayHeader', {
+        defaultMessage: 'Delay',


Is there any reason we weren't showing event loop delay metrics?

Just forgot to add them, likely.

TinaHeiligers · 2021-12-12T23:35:07Z

src/core/public/core_app/status/components/metric_tiles.tsx

-      description={name}
+    <EuiStat
+      data-test-subj={testSubjectName}
+      title={title}


We can only change the text size of the title prop, so I'm using the title to render the important info (values) in the footer.

TinaHeiligers · 2021-12-12T23:37:57Z

src/core/public/core_app/status/components/metric_tiles.tsx

+          metric.meta && (
+            <MetricCardFooter
+              testSubjectName="serverMetricMeta"
+              title={formatDelayFooterTitle(metric.meta.value as number[], metric.meta.type)}


What we want to show in the footer depends on the metric. For event loop delay, it's the percentiles, for the Load it's the load average intervals and in the case of the response times, we're showing the max to give more context to what the mean is. I'm using inline formatting to extract text from the info we want to show. We could, theoretically, do all the formatting in load_status but I chose to do the formatting in the component because it feels more relevant here.

Do you need to cast metric.meta.value as number[] if you do something like metric.meta?.value && (<MetricCardFooter... ?

We could, theoretically, do all the formatting in load_status but I chose to do the formatting in the component because it feels more relevant here.

Yeah we were mixing concerns here already anyway -- some formatting happens in load_status, some in the component. It would be nice to have metric preformatted so that you don't need any conditional logic in the component, but I don't have particularly strong feelings on it.

Value is value: number | number[];, so it has to be force-casted.

I think we should really get rid of this formatMetrics function that converts a properly typed object from the server to this untyped Metric array. It maybe made sense previously as we wanted to have a generic component to ~~rule them all~~ handle all metrics in the same way, but now that we have specific display and component depending on the metric, I think formatMetrics is just making things worse.

That's quite a lot of additional changes though, so we probably don't want to do this in current PR.

That's quite a lot of additional changes though, so we probably don't want to do this in current PR.

We want this to go into 7.17 as a bug fix (forgetting to add the event_loop_delay metrics), so keeping the changes in this PR small is the way to go. It's "only" adding the metrics we didn't have, hence, fixing a bug.

I have the feeling the status page is going to evolve a lot and probably (hopefully) soon to add more detail to the status info at least. We'll need to get design in here to help us figure out the best way we can make this page more useful for diagnosing issues. Once that redesign is done, we can go ahead and refactor.

@lukeelmers @pgayvallet I've made the changes and errored on the side of caution, keeping the changes very small. Would you mind having another look please?

TinaHeiligers · 2021-12-12T23:44:59Z

src/core/public/core_app/status/components/metric_tiles.tsx

@@ -46,3 +121,10 @@ const formatMetric = ({ value, type }: Metric) => {
 const formatMetricId = ({ name }: Metric) => {
  return name.toLowerCase().replace(/[ ]+/g, '-');
 };
+
+const formatDelayFooterTitle = (values: number[], type?: DataType) => {


Having this formatting in-line in the component makes the component code messy to read. We could eventually extract all the formatting helpers into their own file if we end up adding a lot of them.

TinaHeiligers · 2021-12-12T23:48:30Z

src/core/public/core_app/status/lib/load_status.ts

+            defaultMessage: 'Percentiles',
+          }
+        ),
+        title: '',


We're using the value as the title for the Delay card.

TinaHeiligers · 2021-12-12T23:49:19Z

src/core/public/core_app/status/lib/load_status.ts

+          defaultMessage: 'Response time max',
+        }),
+        title: '',
+        value: [metrics.response_times.max_in_millis],


Similar to Delay, I'm using the value as the title in the card footer.

elasticmachine · 2021-12-13T15:37:21Z

Pinging @elastic/kibana-core (Team:Core)

TinaHeiligers · 2021-12-13T15:40:30Z

@elasticmachine merge upstream

lukeelmers · 2021-12-13T23:04:32Z

src/core/public/core_app/status/components/metric_tiles.tsx

+          metric.meta && (
+            <MetricCardFooter
+              testSubjectName="serverMetricMeta"
+              title={formatDelayFooterTitle(metric.meta.value as number[], metric.meta.type)}


Do you need to cast metric.meta.value as number[] if you do something like metric.meta?.value && (<MetricCardFooter... ?

We could, theoretically, do all the formatting in load_status but I chose to do the formatting in the component because it feels more relevant here.

Yeah we were mixing concerns here already anyway -- some formatting happens in load_status, some in the component. It would be nice to have metric preformatted so that you don't need any conditional logic in the component, but I don't have particularly strong feelings on it.

lukeelmers · 2021-12-13T23:10:05Z

src/core/public/core_app/status/components/metric_tiles.tsx

-export const MetricTile: FunctionComponent<{ metric: Metric }> = ({ metric }) => {
-  const { name } = metric;
+export const MetricCardFooter: FunctionComponent<{
+  testSubjectName: string;


nit: you can probably call this data-test-subj directly (as EUI does)

I thought it wasn't possible to spread such keys, but in case someone else though so, TIL:

const foo = { 'data-test-subj': 'bar' }; const { 'data-test-subj': dataTestSub } = foo;

lukeelmers · 2021-12-13T23:11:57Z

src/core/public/core_app/status/components/metric_tiles.tsx

+        footer={
+          metric.meta && (
+            <MetricCardFooter
+              testSubjectName="serverMetricMeta"


I think we usually try to make data-test-subj unique on the page, so maybe we should do something like serverMetricMeta-${formatMetricId(metric)} as was done in EuiCard above?

I had to go with a single data-test-subj for the MetricCardFooter component, in the same way as we use the static "serverMetric" for the flex items.

I think we usually try to make data-test-subj unique on the page

It depends, we usually do when we need to be able to select each node individually from FTR tests. Some elements have shared testSubj id depending on the needs (e.g find all rows from a table).

So it's probably fine. But just to understand:

I had to go with a single data-test-subj for the MetricCardFooter component

Why were you forced to do it? Can't we go with serverMetric-${formatMetricId(metric)}-footer or something?

I wasn't so much forced as wanting to KISS the functional test and use testSubjects.findAll.

lukeelmers · 2021-12-13T23:12:41Z

src/core/public/core_app/status/components/metric_tiles.tsx

+ */
+export const MetricTile: FunctionComponent<{ metric: Metric }> = ({ metric }) => {
+  const { name } = metric;
+  if (name === 'Delay') {


nit: I'd probably go with a switch here for readability, falling back to a default

I think I would even create sub-components for each of the cases and just have this one behaves as a dispatcher.

export const MetricTile: FunctionComponent<{ metric: Metric }> = ({ metric }) => { switch(metric.name) { case 'Delay': return <DelayMetricTile ... /> case 'Load': return <LoadMetricTile ... /> // .... } }

That's not mandatory though, but probably more readable and isolated.

src/core/public/core_app/status/components/metric_tiles.tsx

pgayvallet · 2021-12-15T13:33:20Z

src/core/public/core_app/status/components/metric_tiles.test.tsx

+  it('correctly displays a metric with metadata', () => {
+    const component = shallow(<MetricTile metric={metricWithMeta} />);
+    expect(component).toMatchSnapshot();
+  });


(thinking out loud, unrelated to the PR, don't mind me) snapshot testing was without any doubt the worse thing React did bring to the javascript ecosystem. This thing is a plague.

This thing is a plague.

Ditto to that.

pgayvallet · 2021-12-15T13:37:18Z

src/core/public/core_app/status/components/metric_tiles.tsx

-export const MetricTile: FunctionComponent<{ metric: Metric }> = ({ metric }) => {
-  const { name } = metric;
+export const MetricCardFooter: FunctionComponent<{
+  testSubjectName: string;


I thought it wasn't possible to spread such keys, but in case someone else though so, TIL:

const foo = { 'data-test-subj': 'bar' }; const { 'data-test-subj': dataTestSub } = foo;

pgayvallet · 2021-12-15T13:42:53Z

src/core/public/core_app/status/components/metric_tiles.tsx

+ */
+export const MetricTile: FunctionComponent<{ metric: Metric }> = ({ metric }) => {
+  const { name } = metric;
+  if (name === 'Delay') {


I think I would even create sub-components for each of the cases and just have this one behaves as a dispatcher.

export const MetricTile: FunctionComponent<{ metric: Metric }> = ({ metric }) => { switch(metric.name) { case 'Delay': return <DelayMetricTile ... /> case 'Load': return <LoadMetricTile ... /> // .... } }

That's not mandatory though, but probably more readable and isolated.

pgayvallet · 2021-12-15T13:47:26Z

src/core/public/core_app/status/components/metric_tiles.tsx

+        footer={
+          metric.meta && (
+            <MetricCardFooter
+              testSubjectName="serverMetricMeta"


I think we usually try to make data-test-subj unique on the page

It depends, we usually do when we need to be able to select each node individually from FTR tests. Some elements have shared testSubj id depending on the needs (e.g find all rows from a table).

So it's probably fine. But just to understand:

I had to go with a single data-test-subj for the MetricCardFooter component

Why were you forced to do it? Can't we go with serverMetric-${formatMetricId(metric)}-footer or something?

pgayvallet · 2021-12-15T13:52:34Z

src/core/public/core_app/status/components/metric_tiles.tsx

+          metric.meta && (
+            <MetricCardFooter
+              testSubjectName="serverMetricMeta"
+              title={formatDelayFooterTitle(metric.meta.value as number[], metric.meta.type)}


Value is value: number | number[];, so it has to be force-casted.

I think we should really get rid of this formatMetrics function that converts a properly typed object from the server to this untyped Metric array. It maybe made sense previously as we wanted to have a generic component to ~~rule them all~~ handle all metrics in the same way, but now that we have specific display and component depending on the metric, I think formatMetrics is just making things worse.

That's quite a lot of additional changes though, so we probably don't want to do this in current PR.

pgayvallet · 2021-12-15T13:55:49Z

src/core/public/core_app/status/lib/load_status.ts

-      name: i18n.translate('core.statusPage.metricsTiles.columns.resTimeAvgHeader', {
-        defaultMessage: 'Response time avg',
+      name: i18n.translate('core.statusPage.metricsTiles.columns.processDelayHeader', {
+        defaultMessage: 'Delay',


Just forgot to add them, likely.

TinaHeiligers · 2021-12-15T17:16:43Z

Why were you forced to do it? Can't we go with serverMetric-${formatMetricId(metric)}-footer or something?

Yes, we can go with the unique test-subj but I just wanted to keep it simple and use findAll for that.

kibana-ci · 2021-12-15T19:37:41Z

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`core`	275.7KB	277.6KB	+1.9KB

History

💚 Build #12892 succeeded 1319823
💚 Build #12714 succeeded 6a60e27
💔 Build #12713 failed 0388b07
💚 Build #12709 succeeded b7ddc46
💔 Build #12708 failed 95d488a

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

lukeelmers

Updates LGTM!

Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-12-15T22:18:29Z

💔 Backport failed

Status	Branch	Result
✅	8.0
❌	7.17	The branch "7.17" is invalid or doesn't exist

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 121052

Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Christiane (Tina) Heiligers <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>

TinaHeiligers added 2 commits December 10, 2021 10:00

Adds event loop delay percentiles to mean delay

e0aef73

Uses custom components for grouping metrics

95d488a

TinaHeiligers added v8.0.0 v8.1.0 Feature:StatusPage Issues related to the Kibana Status Page and APIs v7.17.0 labels Dec 12, 2021

TinaHeiligers commented Dec 12, 2021

View reviewed changes

TinaHeiligers added 2 commits December 12, 2021 12:42

Updates snapshot

b7ddc46

Hardness types

0388b07

TinaHeiligers commented Dec 12, 2021

View reviewed changes

Updates unit test

6a60e27

TinaHeiligers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Dec 13, 2021

TinaHeiligers marked this pull request as ready for review December 13, 2021 15:37

TinaHeiligers requested a review from a team as a code owner December 13, 2021 15:37

TinaHeiligers added release_note:enhancement release_note:fix and removed release_note:enhancement labels Dec 13, 2021

Merge branch 'main' into status-page/event_loop_delay_percentiles

1319823

TinaHeiligers mentioned this pull request Dec 13, 2021

Display event loop delay histogram data in the Status page #120667

Closed

lukeelmers reviewed Dec 13, 2021

View reviewed changes

Handles review comments

36daf53

pgayvallet reviewed Dec 15, 2021

View reviewed changes

TinaHeiligers added 2 commits December 15, 2021 10:17

Merge branch 'main' into status-page/event_loop_delay_percentiles

35baff1

Splits metric tiles into isolated components

02da6ea

lukeelmers approved these changes Dec 15, 2021

View reviewed changes

TinaHeiligers merged commit ebafec2 into elastic:main Dec 15, 2021

TinaHeiligers deleted the status-page/event_loop_delay_percentiles branch December 15, 2021 22:08

TinaHeiligers added the auto-backport Deprecated - use backport:version if exact versions are needed label Dec 15, 2021

kibanamachine mentioned this pull request Dec 15, 2021

[8.0] Adds event loop delay percentiles to mean delay (#121052) #121357

Merged

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Dec 15, 2021

Adds event loop delay percentiles to mean delay (elastic#121052)

dac1832

Co-authored-by: Kibana Machine <[email protected]>

kibanamachine added a commit that referenced this pull request Dec 15, 2021

Adds event loop delay percentiles to mean delay (#121052) (#121357)

a43143a

Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Christiane (Tina) Heiligers <[email protected]>

TinaHeiligers added v7.17.0 and removed v7.17.0 labels Dec 16, 2021

TinaHeiligers added a commit to TinaHeiligers/kibana that referenced this pull request Dec 16, 2021

Adds event loop delay percentiles to mean delay (elastic#121052)

42d8868

Co-authored-by: Kibana Machine <[email protected]>

TinaHeiligers mentioned this pull request Dec 16, 2021

[7.17] Adds event loop delay percentiles to mean delay (#121052) #121432

Merged

TinaHeiligers added a commit that referenced this pull request Dec 16, 2021

Adds event loop delay percentiles to mean delay (#121052) (#121432)

21d1296

Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021

Adds event loop delay percentiles to mean delay (elastic#121052)

5abb1c2

Co-authored-by: Kibana Machine <[email protected]>

Adds event loop delay percentiles to mean delay #121052

Adds event loop delay percentiles to mean delay #121052

Conversation

TinaHeiligers commented Dec 12, 2021 • edited Loading

Before:

After:

Checklist

For maintainers

Release Note:

TinaHeiligers Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Dec 12, 2021 • edited Loading

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

TinaHeiligers Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 13, 2021

TinaHeiligers commented Dec 13, 2021

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

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

TinaHeiligers commented Dec 15, 2021

kibana-ci commented Dec 15, 2021

💚 Build Succeeded

Metrics [docs]

Page load bundle

History

lukeelmers left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 15, 2021

💔 Backport failed

TinaHeiligers commented Dec 12, 2021 •

edited

Loading

TinaHeiligers Dec 12, 2021 •

edited

Loading

TinaHeiligers Dec 12, 2021 •

edited

Loading

TinaHeiligers Dec 12, 2021 •

edited

Loading

TinaHeiligers Dec 12, 2021 •

edited

Loading

TinaHeiligers Dec 12, 2021 •

edited

Loading

TinaHeiligers Dec 12, 2021 •

edited

Loading

TinaHeiligers Dec 12, 2021 •

edited

Loading