Stronger typing for monitoring configs #125467

matschaffer · 2022-02-14T07:57:46Z

Summary

This isolates MonitoringConfigSchema (from kibana.yml) from MonitoringConfig (the createConfig return). This allows usage of MonitoringConfig to properly type config access in the rest of the plugin.

To use it, I've replaced all the various formations of config.get => string | undefined around the monitoring plugin.

It's kind of a deep cut, but I didn't see a good way "upgrade" the config.get calls to be properly typed.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

This isolates `MonitoringConfigSchema` (from kibana.yml) from `MonitoringConfig` (the `createConfig` return). This should allow usage of `MonitoringConfig` to properly type config access in the rest of the plugin.

matschaffer · 2022-02-15T01:27:44Z

@elasticmachine merge upstream

kibanamachine · 2022-02-15T01:27:56Z

expected head sha didn’t match current head ref.

Additionally, a few cases where we retrieved `server.uuid` were replaced with a new `instanceUuid` field on LegacyServer.

matschaffer · 2022-02-15T06:28:51Z

x-pack/plugins/monitoring/server/types.ts

@@ -257,3 +250,5 @@ export interface PipelineVersion {
  lastSeen: number;
  hash: string;
 }
+
+export type MonitoringConfigSchema = TypeOf<typeof configSchema>;


I had a hard time deciding what to call this. If I follow core it would be called MonitoringConfigType (

kibana/src/core/server/elasticsearch/elasticsearch_config.ts

Line 22 in 1eea7a3

export type ElasticsearchConfigType = TypeOf<typeof configSchema>;

) but that seemed like a mismatch with the typeof input (configSchema).

As far as I can see, we need to isolate MonitoringConfigSchema (the input config) from MonitoringConfig (the config used across the app) due to the creation of a MonitoringElasticsearchConfig object at ui.elasticsearch in the structure.

Perhaps this is less critical since the code mainly refers to MonitoringConfig?

👍🏻 I like that logic

matschaffer · 2022-02-15T06:40:48Z

@elasticmachine merge upstream

elasticmachine · 2022-02-15T06:42:02Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

matschaffer · 2022-02-15T06:59:22Z

Ah, tests will need a fair bit of updating as well. Moving back to draft while I work on that.

miltonhultgren

Much better! So much accidental complexity removed, and the domain is now more expressive 👏🏼

miltonhultgren · 2022-02-15T11:03:22Z

x-pack/plugins/monitoring/server/types.ts

@@ -257,3 +250,5 @@ export interface PipelineVersion {
  lastSeen: number;
  hash: string;
 }
+
+export type MonitoringConfigSchema = TypeOf<typeof configSchema>;


Perhaps this is less critical since the code mainly refers to MonitoringConfig?

miltonhultgren · 2022-02-15T11:10:02Z

x-pack/plugins/monitoring/server/plugin.ts

@@ -370,9 +347,10 @@ export class MonitoringPlugin
              }
            },
            server: {
+              instanceUuid: this.legacyShimDependencies.instanceUuid,


Nice. Much better to put it under a dedicated key!

matschaffer · 2022-02-16T01:50:02Z

Thanks @miltonhultgren ! Now to just get the api tests to agree (and me remembering how to run the tests again 🤣 )

* Read from metricbeat-* for node_stats * Fix tests Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

A number of these were in JS files which also had things like incorrect arg counts on function calls, so I fixed those up too.

matschaffer · 2022-02-16T04:48:06Z

@elasticmachine merge upstream

kibanamachine · 2022-02-16T04:48:18Z

expected head sha didn’t match current head ref.

…ount

kibana-ci · 2022-02-16T10:34:25Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Default CI Group #21 / apps transform basic license transform permissions for user with full transform access with data loaded should display controls in the edit flyout correctly
[job] [logs] Default CI Group #1 / Execution context Browser apps dashboard app propagates context for Vertical bar visualization
[job] [logs] Default CI Group #15 / machine learning model management trained models for ML user with read-only access renders expanded row content correctly for model with pipelines

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
`monitoring`	10	9	-1

Async chunks

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

id	before	after	diff
`monitoring`	471.5KB	471.4KB	-104.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`monitoring`	2	1	-1

Unknown metric groups

API count

id	before	after	diff
`monitoring`	10	11	+1

History

💚 Build #24519 succeeded ed81f9a
💔 Build #24129 failed 5064cbc
💔 Build #24121 failed 36d0b46
💔 Build #24107 failed c7fa1f9
💛 Build #23768 was flaky 9e6fa77

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

cc @matschaffer

kibanamachine · 2022-02-16T12:41:21Z

The following labels were identified as gaps in your version labels and will be added automatically:

v8.2.0

If any of these should not be on your pull request, please manually remove them.

Replaces previous `config.get` pattern typed to `string | undefined` with a full `MonitoringConfig` type. (cherry picked from commit de206fb)

kibanamachine · 2022-02-16T12:44:03Z

💚 All backports created successfully

Status	Branch	Result
✅	8.1

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

Replaces previous `config.get` pattern typed to `string | undefined` with a full `MonitoringConfig` type. (cherry picked from commit de206fb) Co-authored-by: Mat Schaffer <mat@elastic.co>

matschaffer added 2 commits February 14, 2022 16:55

Stronger typing for monitoring configs

4bcee04

This isolates `MonitoringConfigSchema` (from kibana.yml) from `MonitoringConfig` (the `createConfig` return). This should allow usage of `MonitoringConfig` to properly type config access in the rest of the plugin.

Remove unnecessary export

9e6fa77

matschaffer self-assigned this Feb 14, 2022

matschaffer added the ci:deploy-cloud label Feb 15, 2022

Merge branch 'main' into 112146-sm-config-typing

c7fa1f9

matschaffer added 4 commits February 15, 2022 15:05

Replace legacy config usage with typed MonitoringConfig

7384917

Additionally, a few cases where we retrieved `server.uuid` were replaced with a new `instanceUuid` field on LegacyServer.

Remove unnecessary type parameter

53aeb31

Spelling/grammar

f4b1534

Update one more config case to use the typed interface

b067b2a

matschaffer commented Feb 15, 2022

View reviewed changes

Add note about getConfigCcs removal

b23ff9d

matschaffer marked this pull request as ready for review February 15, 2022 06:40

matschaffer requested a review from a team as a code owner February 15, 2022 06:40

Merge branch 'main' into 112146-sm-config-typing

36d0b46

matschaffer added auto-backport Deprecated - use backport:version if exact versions are needed Feature:Stack Monitoring release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.1.0 labels Feb 15, 2022

matschaffer marked this pull request as draft February 15, 2022 06:59

Update tests that reference update config/server data

5064cbc

miltonhultgren approved these changes Feb 15, 2022

View reviewed changes

matschaffer referenced this pull request Feb 16, 2022

[Monitoring] Read from metricbeat-* for ES node_stats (#76015)

d2ad7ce

* Read from metricbeat-* for node_stats * Fix tests Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Fixing additional places we had old-style config references

4191f04

A number of these were in JS files which also had things like incorrect arg counts on function calls, so I fixed those up too.

matschaffer marked this pull request as ready for review February 16, 2022 04:48

Merge branch 'main' into 112146-sm-config-typing

ed81f9a

Add comments to avoid increasing our "Public APIs missing comments" c…

3cf113a

…ount

matschaffer merged commit de206fb into elastic:main Feb 16, 2022

matschaffer deleted the 112146-sm-config-typing branch February 16, 2022 12:41

kibanamachine added the backport:skip This commit does not require backporting label Feb 16, 2022

kibanamachine added the v8.2.0 label Feb 16, 2022

kibanamachine mentioned this pull request Feb 16, 2022

[8.1] Stronger typing for monitoring configs (#125467) #125776

Merged

matschaffer mentioned this pull request Feb 21, 2022

Quicker option for running per-module doc stats #126037

Open

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stronger typing for monitoring configs #125467

Stronger typing for monitoring configs #125467

matschaffer commented Feb 14, 2022 •

edited

Loading

matschaffer commented Feb 15, 2022

kibanamachine commented Feb 15, 2022

matschaffer Feb 15, 2022

miltonhultgren Feb 15, 2022

matschaffer Feb 16, 2022

matschaffer commented Feb 15, 2022

elasticmachine commented Feb 15, 2022

matschaffer commented Feb 15, 2022

miltonhultgren left a comment

miltonhultgren Feb 15, 2022

miltonhultgren Feb 15, 2022

matschaffer commented Feb 16, 2022

matschaffer commented Feb 16, 2022

kibanamachine commented Feb 16, 2022

kibana-ci commented Feb 16, 2022

API count

kibanamachine commented Feb 16, 2022

kibanamachine commented Feb 16, 2022

Stronger typing for monitoring configs #125467

Stronger typing for monitoring configs #125467

Conversation

matschaffer commented Feb 14, 2022 • edited Loading

Summary

Checklist

For maintainers

matschaffer commented Feb 15, 2022

kibanamachine commented Feb 15, 2022

matschaffer Feb 15, 2022

Choose a reason for hiding this comment

miltonhultgren Feb 15, 2022

Choose a reason for hiding this comment

matschaffer Feb 16, 2022

Choose a reason for hiding this comment

matschaffer commented Feb 15, 2022

elasticmachine commented Feb 15, 2022

matschaffer commented Feb 15, 2022

miltonhultgren left a comment

Choose a reason for hiding this comment

miltonhultgren Feb 15, 2022

Choose a reason for hiding this comment

miltonhultgren Feb 15, 2022

Choose a reason for hiding this comment

matschaffer commented Feb 16, 2022

matschaffer commented Feb 16, 2022

kibanamachine commented Feb 16, 2022

kibana-ci commented Feb 16, 2022

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

Public APIs missing comments

Async chunks

Public APIs missing exports

API count

History

kibanamachine commented Feb 16, 2022

kibanamachine commented Feb 16, 2022

💚 All backports created successfully

Questions ?

matschaffer commented Feb 14, 2022 •

edited

Loading