[core.metrics] Add support for multiple processes in ops metrics & stats API; deprecate process field #109820

Bamieh · 2021-08-24T11:51:00Z

Summary

Deprecate process field
Decided not to add a name field for each process since we do not support multi-processes yet.
Move event loop delay histogram to core
Telemetry collectors to use core's histogram
Core.metrics use histogram.mean for event_loop_delay.
Core.metrics expose histogram to be used for stats and status api.
Updated histogram collection to include inistance_uuid since docker containers will always assign pid: 1 to the kibana process. This fixes a bug for multi kibana instances running on docker machines, or in the rare case more than one server assigns the kibana process the same pid.
Update unit tests
Add integration tests

`/api/status`

{
  "processes": 
     [
	    {
	      "memory": {
	        "heap": {
	          "total_in_bytes": 387874816,
	          "used_in_bytes": 332095280,
	          "size_limit": 12935233536
	        },
	        "resident_set_size_in_bytes": 540487680
	      },
	      "pid": 29897,
	      "event_loop_delay": 11417136.036613273,
	      "event_loop_delay_histogram": {
	        "min": 9469952,
	        "max": 19021823,
	        "mean": 11417136.036613273,
	        "exceeds": 0,
	        "stddev": 1132985.4515301601,
	        "fromTimestamp": "2021-08-09T12:24:51.190Z",
	        "lastUpdatedAt": "2021-08-09T12:24:56.185Z",
	        "percentiles": {
	          "50": 11247615,
	          "75": 12599295,
	          "95": 12656639,
	          "99": 14434303
	        }
	      },
	      "uptime_in_millis": 101736.272458
	    }
	  ],
  "process": {
      "memory": {
        "heap": {
          "total_in_bytes": 387874816,
          "used_in_bytes": 332095280,
          "size_limit": 12935233536
        },
        "resident_set_size_in_bytes": 540487680
      },
      "pid": 29897,
      "event_loop_delay": 11417136.036613273,
      "event_loop_delay_histogram": {
        "min": 9469952,
        "max": 19021823,
        "mean": 11417136.036613273,
        "exceeds": 0,
        "stddev": 1132985.4515301601,
        "fromTimestamp": "2021-08-09T12:24:51.190Z",
        "lastUpdatedAt": "2021-08-09T12:24:56.185Z",
        "percentiles": {
          "50": 11247615,
          "75": 12599295,
          "95": 12656639,
          "99": 14434303
        }
      },
      "uptime_in_millis": 101736.272458
    }
}

Closes #104031
Rebased changes from #107900 (there was a weird issue happening when running TS type checker on that branch).

Bamieh · 2021-08-26T12:03:51Z

@elasticmachine merge upstream

src/core/server/server.api.md

...llection/server/collectors/event_loop_delays/rollups/integration_tests/daily_rollups.test.ts

src/plugins/kibana_usage_collection/server/collectors/ops_stats/ops_stats_collector.test.ts

…multi_process

src/core/server/metrics/collectors/types.ts

…multi_process

… into core/process_multi_process

src/core/server/deprecations/deprecations_service.ts

docs/development/core/server/kibana-plugin-core-server.deprecationsservicesetup.md

src/core/server/metrics/collectors/types.ts

pgayvallet · 2021-09-13T09:25:12Z

src/core/server/metrics/collectors/process.ts

+  public collect(): OpsProcessMetrics[] {
+    return [this.getCurrentPidMetrics()];
+  }


(just thinking out loud, please ignore) I know this array format is better for the downstream indexation, but I'm still not a big fan compared to a Record<workerId, Data>

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

pgayvallet · 2021-09-13T09:30:10Z

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

+        "fromTimestamp",
+        "lastUpdatedAt",
+        "percentiles",


Not sure how far we want to go with unit testing (do we have IT test for this?), but maybe having the mocked monitorEventLoopDelay returning a mocked value to assert that we're properly using/mapping the correct values to our structure would make sense?

Youre right the wiring of this function is not tested. I've added a unit test to ensure that.

src/core/server/metrics/ops_metrics_collector.ts

src/core/server/metrics/types.ts

pgayvallet · 2021-09-13T09:35:09Z

src/core/server/metrics/types.ts

+  max: number;
+  // The mean of the recorded event loop delays.
+  mean: number;
+  // The number of times the event loop delay exceeded the maximum 1 hour event loop delay threshold.


This is taken from https://nodejs.org/api/perf_hooks.html#perf_hooks_histogram_exceeds

I've opened an issue in the node repo to clarify this metric and my understanding of it: nodejs/node#40096

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

pgayvallet · 2021-09-13T09:42:24Z

src/plugins/kibana_usage_collection/server/collectors/ops_stats/ops_stats_collector.ts

    // @ts-expect-error
    delete metrics.process.pid;
+    for (const process of metrics.processes) {
+      // @ts-expect-error
+      delete process.pid;
+    }


Not directly related to the PR as this was done already, but I see we're cloneDeep'ing the metrics just to remove a few fields. Can't we avoid this costly call with just spreads and/or omits?

i agree we can improve this logic. I'd suggest we do it in a different RP though

src/plugins/kibana_usage_collection/server/plugin.ts

…multi_process

Bamieh · 2021-09-13T15:28:54Z

@elasticmachine merge upstream

kibanamachine · 2021-09-13T17:48:24Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`core`	997	1006	+9

Unknown metric groups

API count

id	before	after	diff
`core`	2249	2265	+16

References to deprecated APIs

id	before	after	diff
`kibanaUsageCollection`	0	1	+1
`monitoring`	1	2	+1
total			+2

History

💔 Build #152821 failed 0709677
💚 Build #152696 succeeded c5ba74a
💔 Build #152695 failed a1c48fe
💚 Build #148878 succeeded a7ae5dc
💚 Build #148118 succeeded 939031b

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

lukeelmers

Added a couple small questions, but overall the updates LGTM!

lukeelmers · 2021-09-14T03:53:19Z

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

+ * Side Public License, v 1.
+ */
+
+/* eslint-disable dot-notation */


Curious, why disabling dot-notation here?

Testing private methods must be accessed by bracket notation which is not allowed by eslint in the project. example eventLoopDelaysMonitor['loopMonitor'].enable

lukeelmers · 2021-09-14T03:55:22Z

src/core/server/metrics/types.ts

-  /** Process related metrics */
+  /**
+   * Process related metrics.
+   * @deprecated use the processes field instead.


Should we add a @removeBy for the new docs system? (Not sure if it works here)

lukeelmers · 2021-09-14T04:04:25Z

src/core/server/metrics/types.ts

+ */
+export interface IntervalHistogram {
+  // The first timestamp the interval timer kicked in for collecting data points.
+  fromTimestamp: string;


nit: maybe worth commenting that these are iso 8601

pgayvallet

lgtm

…ats API; deprecate process field (elastic#109820) Co-authored-by: Kibana Machine <[email protected]>

kibanamachine · 2021-09-14T13:54:00Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…ats API; deprecate process field (#109820) (#112108) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Ahmad Bamieh <[email protected]>

rebase changes from elastic#107900

939031b

Bamieh added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Aug 24, 2021

Bamieh requested a review from a team as a code owner August 24, 2021 11:51

Bamieh mentioned this pull request Aug 24, 2021

[core.metrics] Add support for multiple processes in ops metrics & stats API; deprecate process field #107900

Closed

8 tasks

Merge branch 'master' into core/process_multi_process

a7ae5dc

afharo reviewed Aug 26, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into core/process_…

a8a8f90

…multi_process

lukeelmers reviewed Sep 2, 2021

View reviewed changes

src/core/server/metrics/collectors/types.ts Outdated Show resolved Hide resolved

Bamieh added 6 commits September 11, 2021 13:54

pass instanceUuid

077b170

Merge branch 'master' of github.com:elastic/kibana into core/process_…

e366dc4

…multi_process

revert instance uuid in process metric

05b2995

update event loop delays usage collection to include instance_uuid

b009fca

Merge branch 'core/process_multi_process' of github.com:Bamieh/kibana…

041fd60

… into core/process_multi_process

remove extra comment in docs

4ae0511

Bamieh commented Sep 11, 2021

View reviewed changes

src/core/server/deprecations/deprecations_service.ts Show resolved Hide resolved

docs/development/core/server/kibana-plugin-core-server.deprecationsservicesetup.md Show resolved Hide resolved

Bamieh requested a review from lukeelmers September 11, 2021 12:12

Bamieh added 2 commits September 11, 2021 15:20

update documentation

a1c48fe

update telemetry schema

c5ba74a

Bamieh requested a review from a team as a code owner September 11, 2021 14:27

pgayvallet reviewed Sep 13, 2021

View reviewed changes

Bamieh added 2 commits September 13, 2021 15:50

core review fixes

d1de70a

Merge branch 'master' of github.com:elastic/kibana into core/process_…

0709677

…multi_process

Bamieh requested a review from pgayvallet September 13, 2021 12:51

Merge branch 'master' into core/process_multi_process

50b844d

lukeelmers approved these changes Sep 14, 2021

View reviewed changes

pgayvallet approved these changes Sep 14, 2021

View reviewed changes

Bamieh merged commit b74f154 into elastic:master Sep 14, 2021

Bamieh deleted the core/process_multi_process branch September 14, 2021 13:50

kibanamachine mentioned this pull request Sep 14, 2021

[7.x] [core.metrics] Add support for multiple processes in ops metrics & stats API; deprecate process field (#109820) #112108

Merged

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 14, 2021

[core.metrics] Add support for multiple processes in ops metrics & st…

5776cb8

…ats API; deprecate process field (elastic#109820) Co-authored-by: Kibana Machine <[email protected]>

matschaffer mentioned this pull request Nov 2, 2021

[8.0] Kibana GET /api/stats breaking changes elastic/beats#27569

Open

TinaHeiligers mentioned this pull request Nov 13, 2021

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

Merged

1 task

TinaHeiligers mentioned this pull request Dec 7, 2021

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core.metrics] Add support for multiple processes in ops metrics & stats API; deprecate process field #109820

[core.metrics] Add support for multiple processes in ops metrics & stats API; deprecate process field #109820

Bamieh commented Aug 24, 2021 •

edited

Loading

Bamieh commented Aug 26, 2021

pgayvallet Sep 13, 2021

pgayvallet Sep 13, 2021

Bamieh Sep 13, 2021

pgayvallet Sep 13, 2021

Bamieh Sep 13, 2021

pgayvallet Sep 13, 2021

Bamieh Sep 13, 2021

Bamieh commented Sep 13, 2021

kibanamachine commented Sep 13, 2021

API count

References to deprecated APIs

lukeelmers left a comment

lukeelmers Sep 14, 2021

Bamieh Sep 14, 2021

lukeelmers Sep 14, 2021

lukeelmers Sep 14, 2021

pgayvallet left a comment

kibanamachine commented Sep 14, 2021

[core.metrics] Add support for multiple processes in ops metrics & stats API; deprecate process field #109820

[core.metrics] Add support for multiple processes in ops metrics & stats API; deprecate process field #109820

Conversation

Bamieh commented Aug 24, 2021 • edited Loading

Summary

/api/status

Bamieh commented Aug 26, 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

Bamieh commented Sep 13, 2021

kibanamachine commented Sep 13, 2021

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

References to deprecated APIs

History

lukeelmers 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

pgayvallet left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 14, 2021

💚 Backport successful

Bamieh commented Aug 24, 2021 •

edited

Loading

`/api/status`