[Infra] Replace infra aggregation runtime types with the types in `estypes` #192059

miloszmarcinkowski · 2024-09-04T07:47:05Z

Closes #190497

Summary

This PR replaces infra aggregation types with types provided by the Elasticsearch client.

Testing

Alerting tested manually.

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

obltmachine · 2024-09-04T07:47:17Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

jennypavlova

I know it's still a draft, I am just adding some questions.

x-pack/plugins/observability_solution/metrics_data_access/common/inventory_models/types.ts

miloszmarcinkowski · 2024-09-04T11:25:08Z

Hey @crespocarlos

I see you had some good comments in similar PR #190495 therefore could you please take a look at this one?

Questions:

I have been thinking about moving type guards (isBasicMetricAgg, isDerivativeAgg, isSumBucketAgg) outside types.ts and putting them in a new file type_guards.ts. Wdyt?
Is there anything left to replace with estypes?

crespocarlos

Nice one! Left a few comments.

crespocarlos · 2024-09-04T12:16:25Z

x-pack/plugins/observability_solution/metrics_data_access/common/inventory_models/types.ts

+  if (agg && typeof agg === 'object' && Object.keys(agg).length !== 0) {
+    for (const key in agg) {
+      if (Object.hasOwn(agg, key)) {
+        return !!(
+          agg[key as keyof typeof agg] === undefined ||
+          (agg[key as keyof typeof agg] as estypes.AggregationsMetricAggregationBase).field
+        );
+      }
+    }
+  }


Tip
Maybe this can be simplified?

Suggested change

if (agg && typeof agg === 'object' && Object.keys(agg).length !== 0) {

for (const key in agg) {

if (Object.hasOwn(agg, key)) {

return !!(

agg[key as keyof typeof agg] === undefined ||

(agg[key as keyof typeof agg] as estypes.AggregationsMetricAggregationBase).field

);

}

}

}

if (typeof agg !== 'object' || agg === null) {

return false;

}

return Object.values(agg).some(

value => value === undefined || 'field' in (value as estypes.AggregationsMetricAggregationBase)

);

crespocarlos · 2024-09-04T12:17:29Z

...s/observability_solution/infra/server/lib/alerting/inventory_metric_threshold/lib/is_rate.ts

@@ -8,10 +8,10 @@
 import { has } from 'lodash';


Should we add tests for the functions here?

crespocarlos · 2024-09-04T12:19:04Z

x-pack/plugins/observability_solution/metrics_data_access/common/index.ts

+  isDerivativeAgg,
+  isBasicMetricAgg,
+  isSumBucketAgg,
 } from './inventory_models/types';


Should these be in the types file? I wonder if it would be better if we created a utils file or something like that. wdyt?

crespocarlos · 2024-09-04T12:56:41Z

...s/observability_solution/infra/server/lib/alerting/inventory_metric_threshold/lib/is_rate.ts

@@ -37,7 +37,7 @@ export const isInterfaceRateAgg = (metric: MetricsUIAggregation | undefined) =>
  const values = Object.values(metric);
  return (
    values.some((agg) => ESTermsWithAggregationRT.is(agg)) &&


I forgot this one. Let's try to replace this too.

I had a chat with Jenny earlier today, and we weren't sure about replacing ESTermsWithAggregationRT. I'll do it then. Thanks

crespocarlos · 2024-09-04T12:58:08Z

Hey! I had reviewed it before reading this:

Questions:

I have been thinking about moving type guards (isBasicMetricAgg, isDerivativeAgg, isSumBucketAgg) outside types.ts and putting them in a new file type_guards.ts. Wdyt?

I agree with moving the type guards to another file, I've even left a comment about it already :).

Is there anything left to replace with estypes?

From what I've seen, there is only ESTermsWithAggregationRT left

crespocarlos · 2024-09-04T16:19:46Z

x-pack/plugins/observability_solution/metrics_data_access/common/inventory_models/index.ts

+  return !!(
+    (agg as estypes.AggregationsAggregationContainer).aggregations &&
+    (agg as estypes.AggregationsAggregationContainer).terms
+  );


nit to avoid repetition

Suggested change

return !!(

(agg as estypes.AggregationsAggregationContainer).aggregations &&

(agg as estypes.AggregationsAggregationContainer).terms

);

const container = agg as estypes.AggregationsAggregationContainer;

return !!(container?.terms && container?.aggregations);

elasticmachine · 2024-09-05T09:31:49Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

crespocarlos

LGTM! Thanks for this PR. Left just one nit.

crespocarlos · 2024-09-05T09:46:21Z

...ervability_solution/infra/server/lib/alerting/inventory_metric_threshold/lib/is_rate.test.ts

+
+describe('isRate', () => {
+  describe('isMetricRate', () => {
+    // todo: do we mock outside of file?


it's fine to leave these mocks on this file. do we still need this todo?

Ops. I committed it unintentionally. It was a reminder for me to clarify it with you guys. Updated, thanks!

crespocarlos · 2024-09-05T09:47:41Z

...ervability_solution/infra/server/lib/alerting/inventory_metric_threshold/lib/is_rate.test.ts

@@ -0,0 +1,74 @@
+/*


Thanks for add the test.

jennypavlova

LGTM 💯 Thank you for changing those types, it looks better and we don't need to maintain those types anymore!

kibana-ci · 2024-09-05T11:20:21Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 7aa5f38
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-192059-7aa5f3898d1e

Failed CI Steps

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
`metricsDataAccess`	133	137	+4

Page load bundle

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

id	before	after	diff
`metricsDataAccess`	62.2KB	62.0KB	-157.0B

Unknown metric groups

API count

id	before	after	diff
`metricsDataAccess`	133	137	+4

History

💔 Build #232163 failed 6fa3c47b9ae49a8249acf3ca36b70a0a954a2028
💔 Build #231917 failed 7b7fd57ce4f6847a50019429ec7f3072866a1e64

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

miloszmarcinkowski force-pushed the 190497-replace-infra-aggregation-runtime-types-with-the-types-in-estypes branch from e529ea7 to 7b7fd57 Compare September 4, 2024 10:29

miloszmarcinkowski changed the title ~~replace 'ESBasicMetricAggRT' and 'ESDerivativeAggRT' with 'estypes'~~ [Infra] Replace infra aggregation runtime types with the types in estypes Sep 4, 2024

jennypavlova reviewed Sep 4, 2024

View reviewed changes

x-pack/plugins/observability_solution/metrics_data_access/common/inventory_models/types.ts Outdated Show resolved Hide resolved

x-pack/plugins/observability_solution/metrics_data_access/common/inventory_models/types.ts Outdated Show resolved Hide resolved

miloszmarcinkowski force-pushed the 190497-replace-infra-aggregation-runtime-types-with-the-types-in-estypes branch from db8c5da to 7b7fd57 Compare September 4, 2024 11:23

crespocarlos reviewed Sep 4, 2024

View reviewed changes

miloszmarcinkowski marked this pull request as ready for review September 5, 2024 09:27

miloszmarcinkowski requested review from a team as code owners September 5, 2024 09:27

botelastic bot added the ci:project-deploy-observability Create an Observability project label Sep 5, 2024

shahzad31 approved these changes Sep 5, 2024

View reviewed changes

miloszmarcinkowski added v8.16.0 release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Sep 5, 2024

miloszmarcinkowski added 6 commits September 5, 2024 11:35

replace 'ESBasicMetricAggRT' and 'ESDerivativeAggRT' with 'estypes'

ca5acbd

replace 'ESSumBucketAggRT' with 'estypes'

5d02dcc

improve isBasicMetricAgg method

9838fdf

move type guards from types.ts to index.ts (serves as utils folder)

a6ea051

replace 'ESTermsWithAggregationRT' with 'estype'

19b6ada

avoid repetition in isTermsWithAggregation (improvement)

5a72aa0

crespocarlos approved these changes Sep 5, 2024

View reviewed changes

jennypavlova approved these changes Sep 5, 2024

View reviewed changes

cover 'is_rate' with unit tests

7aa5f38

miloszmarcinkowski force-pushed the 190497-replace-infra-aggregation-runtime-types-with-the-types-in-estypes branch from 20bb82a to 7aa5f38 Compare September 5, 2024 10:23

miloszmarcinkowski merged commit 8afac4d into elastic:main Sep 5, 2024
23 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Infra] Replace infra aggregation runtime types with the types in `estypes` #192059

[Infra] Replace infra aggregation runtime types with the types in `estypes` #192059

miloszmarcinkowski commented Sep 4, 2024 •

edited

Loading

obltmachine commented Sep 4, 2024

jennypavlova left a comment

miloszmarcinkowski commented Sep 4, 2024

crespocarlos left a comment

crespocarlos Sep 4, 2024

crespocarlos Sep 4, 2024 •

edited

Loading

crespocarlos Sep 4, 2024

crespocarlos Sep 4, 2024 •

edited

Loading

miloszmarcinkowski Sep 4, 2024

crespocarlos commented Sep 4, 2024 •

edited

Loading

crespocarlos Sep 4, 2024

elasticmachine commented Sep 5, 2024

crespocarlos left a comment

crespocarlos Sep 5, 2024

miloszmarcinkowski Sep 5, 2024

crespocarlos Sep 5, 2024

jennypavlova left a comment

kibana-ci commented Sep 5, 2024 •

edited

Loading

API count

[Infra] Replace infra aggregation runtime types with the types in estypes #192059

[Infra] Replace infra aggregation runtime types with the types in estypes #192059

Conversation

miloszmarcinkowski commented Sep 4, 2024 • edited Loading

Summary

Testing

Checklist

obltmachine commented Sep 4, 2024

🤖 GitHub comments

jennypavlova left a comment

Choose a reason for hiding this comment

miloszmarcinkowski commented Sep 4, 2024

crespocarlos left a comment

Choose a reason for hiding this comment

crespocarlos Sep 4, 2024

Choose a reason for hiding this comment

crespocarlos Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

crespocarlos Sep 4, 2024

Choose a reason for hiding this comment

crespocarlos Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

miloszmarcinkowski Sep 4, 2024

Choose a reason for hiding this comment

crespocarlos commented Sep 4, 2024 • edited Loading

crespocarlos Sep 4, 2024

Choose a reason for hiding this comment

elasticmachine commented Sep 5, 2024

crespocarlos left a comment

Choose a reason for hiding this comment

crespocarlos Sep 5, 2024

Choose a reason for hiding this comment

miloszmarcinkowski Sep 5, 2024

Choose a reason for hiding this comment

crespocarlos Sep 5, 2024

Choose a reason for hiding this comment

jennypavlova left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 5, 2024 • edited Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

Page load bundle

API count

History

[Infra] Replace infra aggregation runtime types with the types in `estypes` #192059

[Infra] Replace infra aggregation runtime types with the types in `estypes` #192059

miloszmarcinkowski commented Sep 4, 2024 •

edited

Loading

crespocarlos Sep 4, 2024 •

edited

Loading

crespocarlos Sep 4, 2024 •

edited

Loading

crespocarlos commented Sep 4, 2024 •

edited

Loading

kibana-ci commented Sep 5, 2024 •

edited

Loading