Handles ns to ms conversion for event loop delay metrics #118447

TinaHeiligers · 2021-11-12T01:46:04Z

How this PR fixes the issue:

This PR converts the raw data in core's EventLoopDelayMonitor from ns to ms and reports the data in ms. We source the data from Node's native monitorEventLoopDelay histogram, that is in ns.

How was the bug introduced?

Event loop delays collection was moved from the kibana_usage_collection plugin to core. There were several other changes that resulted from this move, including changing the process event_loop_delay to read from the mean of the histogram which uses nanoseconds rather than milliseconds.
Monitoring gets kibana metrics from the status endpoint, which includes core’s metrics.
On setup, monitoring initializes the bulkUploader with metrics from core and gets the kibana stats from there.

How was the metric calculated before?

Prior to this change, we were using bench.elapsed from hapi/hoek.
Reference for bench.elapsed internals: https://github.com/hapijs/hoek/blob/master/lib/bench.js#L19-L22.

Screenshots

Actual numbers from these screen shots will differ due to environment. Review relative orders of magnitude between Event loop delay.

7-15 stack deployment (cloud-staging)

7-16 stack deployment (cloud-staging)

** This PR (running locally) **

PR deployed on cloud here. Use the same auth creds as running this PR locally.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

Risk Matrix

Risk	Probability	Severity	Mitigation/Notes
`event_loop_delay metrics` reported incorrectly.	Low	Medium	Any downstream consumers of metrics data need to ensure that they take metrics data units into account.

Release Note

Fixes bug in event_loop_delay metrics units.

TinaHeiligers

Self review

TinaHeiligers · 2021-11-12T23:56:36Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.mocks.ts

  const now = Date.now();

-  return {
+  const mockedRawCollectedDataInNs = {


Mocks the raw data returned from Node's native API in nanoseconds.

TinaHeiligers · 2021-11-12T23:57:10Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.mocks.ts

+  return mockedRawCollectedDataInNs;
+}
+
+function createMockMonitorDataMsHistogram(


Create mock data in ms returned from the monitor

TinaHeiligers · 2021-11-12T23:58:18Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.mocks.ts


  return new MockEventLoopDelaysMonitor();
 }

 export const mocked = {
-  createHistogram: createMockHistogram,
+  createHistogram: createMockRawNsDataHistogram, // raw data as received from Node.js perf_hooks.monitorEventLoopDelay([options])


createHistogram is used to generate unit test input data that the monitor then converts from ns -> ms.

TinaHeiligers · 2021-11-13T00:00:44Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.test.ts

    // This ensures that the wiring of the `collect` function is correct.
    const mockedHistogram = mocked.createHistogram();
-    expect(histogramData).toEqual(mockedHistogram);
+
+    expect(histogramData.min).toEqual(nsToMs(mockedHistogram.min));


I couldn't come up with an elegant way to test all of this that was still explicit enough for us to glance over and realize there's a unit conversion taking place.

Since the units already threw us once, I'm erring on the side of caution here.

TinaHeiligers · 2021-11-13T00:01:32Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.ts

+ * Converts time metric from ns to ms
+ **/
+export function nsToMs(metric: number) {
+  return moment.duration(metric / ONE_MILLISECOND_AS_NANOSECONDS).asMilliseconds();


I'm torn between using moment here and just treating the number as a number.

Let's use the native API whenever possible. I'd remove moment usage as it doesn't bring any value.

TinaHeiligers · 2021-11-13T00:02:24Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.ts

-      mean,
-      exceeds,
-      stddev,
+    const collectedDataMs: IntervalHistogram = {


This implementation ended up being the easiest to read.

TinaHeiligers · 2021-11-13T00:02:49Z

src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/constants.ts

@@ -32,7 +32,7 @@ export const MONITOR_EVENT_LOOP_DELAYS_RESET = 24 * 60 * 60 * 1000;
 export const MONITOR_EVENT_LOOP_DELAYS_START = 1 * 60 * 1000;

 /**
- * Mean event loop delay threshold for logging a warning.
+ * Mean event loop delay threshold in ms for logging a warning.


Added the unit here as it wasn't explicit.

TinaHeiligers · 2021-11-13T00:03:16Z

src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/schema.ts

@@ -59,19 +59,19 @@ export const eventLoopDelaysUsageSchema: MakeSchemaFrom<EventLoopDelaysUsageRepo
      min: {
        type: 'long',
        _meta: {
-          description: 'The minimum recorded event loop delay.',
+          description: 'The minimum recorded event loop delay in ms.',


Adding units to telemetry field descriptions helps!

TinaHeiligers · 2021-11-13T00:03:51Z

src/plugins/kibana_usage_collection/server/collectors/event_loop_delays/track_threshold.ts

-      const meanDurationMs = moment
-        .duration(mean / ONE_MILLISECOND_AS_NANOSECONDS)
-        .asMilliseconds();
+      const { mean: meanMS } = eventLoopDelaysMonitor.collect();


The data's already in ms by now.

TinaHeiligers · 2021-11-13T17:28:24Z

@elasticmachine merge upstream

TinaHeiligers · 2021-11-14T19:07:56Z

@elasticmachine merge upstream

elasticmachine · 2021-11-15T15:12:34Z

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

pgayvallet

LGTM

mshustov · 2021-11-15T16:28:12Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.ts

+/**
+ * Nanosecond to milisecond conversion unit
+ */
+export const ONE_MILLISECOND_AS_NANOSECONDS = 1000000;


Suggested change

export const ONE_MILLISECOND_AS_NANOSECONDS = 1000000;

export const ONE_MILLISECOND_AS_NANOSECONDS = 1_000_000;

Definitely easier to parse by eye. Nice suggestion!

mshustov · 2021-11-15T16:29:14Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.ts

+ * Converts time metric from ns to ms
+ **/
+export function nsToMs(metric: number) {
+  return moment.duration(metric / ONE_MILLISECOND_AS_NANOSECONDS).asMilliseconds();


Let's use the native API whenever possible. I'd remove moment usage as it doesn't bring any value.

mshustov · 2021-11-15T16:30:09Z

src/core/server/metrics/event_loop_delays/event_loop_delays_monitor.ts

      },
    };

    this.loopMonitor.enable();
-    return collectedData;
+    return collectedDataMs;


nit: not all data in the returned object are in milliseconds.

kibana-ci · 2021-11-15T19:47:21Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #6529 succeeded 82b5206
💚 Build #6522 succeeded 15cd397
💚 Build #6510 succeeded fca6022
💔 Build #6509 failed f0ec458
💔 Build #6508 failed 150c75e
💚 Build #6446 succeeded 5d7e2b7

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

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

kibanamachine · 2021-11-15T19:51:19Z

💚 Backport successful

Status	Branch	Result
✅	8.0
✅	7.16

The backport PRs will be merged automatically after passing CI.

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

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

…igrate-away-from-injected-css-js * 'main' of github.com:elastic/kibana: (221 commits) [Reporting] Add log level to config (elastic#118319) [Metrics UI] Skip failing waffle chart color palette test (elastic#118527) [APM] chore: Unify naming of 'apm/scripts/**/*' with snake_case (elastic#118328) skip flaky suites (elastic#99581, elastic#118356, elastic#118474) [Alerting] Initial implementation of alerting task `cancel()` (elastic#114289) chore(NA): creates pkg_npm_types bazel rule (elastic#116465) skip flaky suite (elastic#116892) Bump chromedriver to 95.0.0 (elastic#116724) [Data visualizer] Improve design of expanded rows (elastic#118125) [APM] prefer ECS field names for HTTP and URL (elastic#118485) Update query_debugging_in_development_and_production.md (elastic#118491) [Uptime] adjust Elastic Synthetics integration functional tests (elastic#118163) [kbn/rule-data-utils] add submodules and require public use them (elastic#117963) [DOCS] Refresh APM correlation screenshots (elastic#116723) (elastic#118577) Handles ns to ms conversion for event loop delay metrics (elastic#118447) [Cases] Rename functional tests folder (elastic#118490) dummy commit skip flaky suite (elastic#118593) Improve workpad schema validation (elastic#97838) skip flaky suite (elastic#118584) ... # Conflicts: # src/plugins/dashboard/public/application/embeddable/viewport/dashboard_viewport.tsx

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

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

Handles ns to ms conversion for event loop delay metrics

a8f94bf

TinaHeiligers added auto-backport Deprecated - use backport:version if exact versions are needed backport-v8.0.0 v7.16.1 v8.1.0 release_note:fix and removed auto-backport Deprecated - use backport:version if exact versions are needed labels Nov 12, 2021

TinaHeiligers added 4 commits November 11, 2021 18:54

removes debugging logs

5d7e2b7

Removes ns -> ms conversion factor from kibana_usage_collection

150c75e

removes unused import and constant

5ce71d4

Adds units to telemetry schema

f0ec458

TinaHeiligers commented Nov 13, 2021

View reviewed changes

updates telemetry schema

fca6022

TinaHeiligers marked this pull request as ready for review November 13, 2021 17:27

TinaHeiligers requested review from a team as code owners November 13, 2021 17:27

Merge branch 'main' into report_event_loop_delay_in_ms

15cd397

TinaHeiligers mentioned this pull request Nov 13, 2021

7.16 event loop delay value incorrect #116778

Closed

Merge branch 'main' into report_event_loop_delay_in_ms

82b5206

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

TinaHeiligers added v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed and removed backport-v8.0.0 labels Nov 15, 2021

pgayvallet approved these changes Nov 15, 2021

View reviewed changes

mshustov approved these changes Nov 15, 2021

View reviewed changes

Addresses review comments

55a613a

TinaHeiligers enabled auto-merge (squash) November 15, 2021 18:20

TinaHeiligers merged commit 19706fb into elastic:main Nov 15, 2021

kibanamachine mentioned this pull request Nov 15, 2021

[8.0] Handles ns to ms conversion for event loop delay metrics (#118447) #118612

Merged

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

Handles ns to ms conversion for event loop delay metrics (elastic#118447

7127d7b

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

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

Handles ns to ms conversion for event loop delay metrics (elastic#118447

1a0a749

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

kibanamachine mentioned this pull request Nov 15, 2021

[7.16] Handles ns to ms conversion for event loop delay metrics (#118447) #118613

Merged

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

Handles ns to ms conversion for event loop delay metrics (#118447) (#…

cca1688

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

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

Handles ns to ms conversion for event loop delay metrics (#118447) (#…

932e4c8

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

TinaHeiligers added v7.16.0 and removed v7.16.1 labels Nov 16, 2021

mbondyra pushed a commit to mbondyra/kibana that referenced this pull request Nov 19, 2021

Handles ns to ms conversion for event loop delay metrics (elastic#118447

45f7468

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

dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021

Handles ns to ms conversion for event loop delay metrics (#118447)

75de43c

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handles ns to ms conversion for event loop delay metrics #118447

Handles ns to ms conversion for event loop delay metrics #118447

TinaHeiligers commented Nov 12, 2021 •

edited

Loading

TinaHeiligers left a comment

TinaHeiligers Nov 12, 2021

TinaHeiligers Nov 12, 2021

TinaHeiligers Nov 12, 2021

TinaHeiligers Nov 13, 2021

TinaHeiligers Nov 13, 2021

mshustov Nov 15, 2021

TinaHeiligers Nov 13, 2021

TinaHeiligers Nov 13, 2021

TinaHeiligers Nov 13, 2021

TinaHeiligers Nov 13, 2021

TinaHeiligers commented Nov 13, 2021

TinaHeiligers commented Nov 14, 2021

elasticmachine commented Nov 15, 2021

pgayvallet left a comment

mshustov Nov 15, 2021

TinaHeiligers Nov 15, 2021

mshustov Nov 15, 2021

mshustov Nov 15, 2021

kibana-ci commented Nov 15, 2021

kibanamachine commented Nov 15, 2021

	export const ONE_MILLISECOND_AS_NANOSECONDS = 1000000;
	export const ONE_MILLISECOND_AS_NANOSECONDS = 1_000_000;

Handles ns to ms conversion for event loop delay metrics #118447

Handles ns to ms conversion for event loop delay metrics #118447

Conversation

TinaHeiligers commented Nov 12, 2021 • edited Loading

How this PR fixes the issue:

How was the bug introduced?

How was the metric calculated before?

Screenshots

Checklist

Risk Matrix

Release Note

TinaHeiligers left a 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 Nov 13, 2021

TinaHeiligers commented Nov 14, 2021

elasticmachine commented Nov 15, 2021

pgayvallet left a 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

kibana-ci commented Nov 15, 2021

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Nov 15, 2021

💚 Backport successful

TinaHeiligers commented Nov 12, 2021 •

edited

Loading