[Infra] Replace `MetricsUIAggregation` in favor of `estypes.AggregationsAggregate` by jennypavlova · Pull Request #190495 · elastic/kibana

jennypavlova · 2024-08-14T10:53:28Z

Closes #190311

Summary

This PR replaces MetricsUIAggregation in favor of estypes.AggregationsAggregate. Now the MetricsUIAggregation uses estypes.AggregationsAggregate and the MetricsUIAggregationRT is removed which also allows us to remove the ESAggregationRT. Instead of maintaining the runtime types we now rely on the types provided by elasticsearch.

A follow-up issue will be linked here to address the other aggregation types related changes: #190497

Testing

Check the types
Host page should load with no errors
To test if the API works use the request provided in the x-pack/plugins/observability_solution/infra/server/routes/infra/README.md - if the body is not valid 400 is returned

obltmachine · 2024-08-14T10:53:45Z

🤖 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 · 2024-08-14T10:54:03Z

/ci

…-favor-of-estypesaggregationsaggregate

jennypavlova · 2024-08-14T14:09:32Z

/ci

…-fix'

jennypavlova · 2024-08-14T15:04:04Z

/ci

crespocarlos

Thanks for this refactoring! I know it's still a draft, but I've left some comments.

Nice that the files in the snapshot folder are already reflecting the changes without having to change them.

crespocarlos · 2024-08-15T07:59:12Z

@@ -6,15 +6,12 @@
 */


Apparently, this endpoint is used nowhere 🤔

Interesting - maybe it was replaced at one point. I will check if I can remove it.

Removed ✅

crespocarlos · 2024-08-15T08:15:26Z

+export const hasAggregations = (
+  aggregations?: Record<string, estypes.AggregationsAggregate>
+): aggregations is Record<string, estypes.AggregationsAggregate> => {
+  return !!(aggregations as Record<string, estypes.AggregationsAggregate>);
+};

-export const MetricsUIAggregationRT = rt.record(rt.string, ESAggregationRT);
-export type MetricsUIAggregation = rt.TypeOf<typeof MetricsUIAggregationRT>;
+export const hasSnapshotTermsWithAggregation = (
+  termWithAggregation?: unknown
+): termWithAggregation is SnapshotTermsWithAggregation => {
+  return !!(termWithAggregation as SnapshotTermsWithAggregation);
+};


Could these be moved to a helper/utils file?

Those are removed now as we use the previous rt types for alerts

crespocarlos · 2024-08-15T08:38:59Z

@@ -5,35 +5,38 @@
 * 2.0.


Can we remove this file? It's a duplicate from metrics_api in metrics_data_access.

Removed ✅

crespocarlos · 2024-08-15T08:40:45Z

+export interface TimeRange {
+  from: number;
+  to: number;
+  interval: string;
+}
+
+export interface MetricsAPIMetric {
+  id: string;
+  aggregations: MetricsUIAggregation;
+}
+
+export interface MetricsAPIRequest {
+  timerange: TimeRange;
+  indexPattern: string;
+  metrics: MetricsAPIMetric[];
+  includeTimeseries: boolean | undefined;
+  groupBy?: Array<string | null | undefined>;
+  groupInstance?: Array<string | null | undefined>;
+  modules?: string[];
+  afterKey?: Record<string, string | null> | null;
+  limit?: number | null;
+  filters?: Array<Record<string, unknown>>;
+  dropPartialBuckets?: boolean;
+  alignDataToEnd?: boolean;
+}


Perhaps we can leave this typed with io-ts, wdyt? We just need to change aggregations to be an rt.UnkonwnRecord.

I wouldn't be so worried about it being an UnknownRecord because /infra/metrics/metrics_api doesn't seem to be used, so this is just used internally by /metrics_explorer and /snapshot endpoints when they call the query function.

/metrics_explorer builds the object manually in the convertMetricToMetricsAPIMetric function and /snapshot, except for custom metrics, gets the aggregations from inventory_models/<entity_type>/snapshot/ folder. Those metrics will be typed correctly

I changed that, thanks for the suggestion!

crespocarlos · 2024-08-15T08:43:52Z


 export type MetricsAPISeries = rt.TypeOf<typeof MetricsAPISeriesRT>;

-export type MetricsAPIRequest = rt.TypeOf<typeof MetricsAPIRequestRT>;


Alternatively, if you decide to keep MetricsAPIRequestRT, you can do the following:

export type MetricsAPIRequest = Omit<rt.OutputOf<typeof MetricsAPIRequestRT>, 'metrics'> & { metrics: Array<MetricsAPIMetric>; // ts type };

It will generate the same typescript type you defined above. This way, internally, anywhere that uses MetricsAPIRequest will be forced to be compatible with MetricsUIAggregation type.

The MetricsAPIRequestRT type in this case is more to validate the request body for the metrics_api endpoint.

Good point! Changed 👍

crespocarlos · 2024-08-15T08:52:16Z

+      if (hasAggregations(aggregations)) {
        return { id: metric.type, aggregations };
      }


If the aggregation comes from snapshot object, which is a catalog of metrics aggregations we maintain, and it's properly typed, I would say we don't need this if. If it's not a SnapshotCustomMetricInputRT.is(metric) we can return { id: metric.type, aggregations } without this check.

Removed ✔️

crespocarlos · 2024-08-15T09:00:08Z

+  if (hasAggregations(metric) || metric === undefined) {
    return false;
  }


Q: Isn't hasAggregations(metric) redundant, considering the metric is already defined as MetricUIAggregation? Perhaps we could leave just the undefined check here?

I left only the undefined check 👍

crespocarlos · 2024-08-15T09:02:46Z

  const values = Object.values(metric);
  return (
-    values.some((agg) => ESTermsWithAggregationRT.is(agg)) &&
+    values.some((agg) => hasSnapshotTermsWithAggregation(agg)) &&


Wdyt if we keep ESTermsWithAggregationRT for alerting stuff?

I was trying that but the issue I faced is that ESTermsWithAggregationRT is still using the aggregation type we replace and if we try to still use it then SnapshotTermsWithAggregation is using already the new MetricsUIAggregation which depends on the estypes (Record<string, estypes.AggregationsAggregate>) and there will be incompatible with the "old" type.
I will think about another way to keep them compatible but so far what I tried was not working

So I used the old es types and SnapshotTermsWithAggregation renamed to SnapshotTermsWithAggregationForAlerts to keep them the same as before but this will also need refactoring in the future I guess

…place-metricsuiaggregation-in-favor-of-estypesaggregationsaggregate

…-favor-of-estypesaggregationsaggregate

elasticmachine · 2024-08-19T17:22:09Z

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

crespocarlos

Hey! Thanks for the applying the suggestions. I've got a few more. Overall, it's looking good.

crespocarlos · 2024-08-19T17:49:22Z

  return metrics.reduce((aggs, metric, index) => {
    const aggregation = metricToAggregation(options.nodeType, metric, index);
-    if (!MetricsUIAggregationRT.is(aggregation)) {
+    if (!!(aggregation as Record<string, estypes.AggregationsAggregate>)) {


I guess here we can just check if aggregation is undefined. The only way for aggregation to not be valid is if the metric doesn't exist in the snapshot object for the nodeType.

it makes sense, changed 👍

crespocarlos · 2024-08-19T17:50:40Z

+export interface TimeRange {
+  from: number;
+  to: number;
+  interval: string;
+}


Is this used? We can remove it in favor of MetricsAPITimerange, wdyt?

Removed, thanks!

crespocarlos · 2024-08-19T17:52:23Z

+export interface MetricsAPIMetric {
+  id: string;
+  aggregations: MetricsUIAggregation;
+}


Do we need to export this?

Yes, it is used as a type in different places.

crespocarlos · 2024-08-19T17:57:43Z

+export interface SnapshotTermsWithAggregationForAlerts {
+  terms: { field: string };
+  aggregations: rt.TypeOf<typeof MetricsUIAggregationRT>;
+}


We could probably remove this at this point if we change the ESTermsWithAggregationRT and ESAggregationRT types as suggested

crespocarlos · 2024-08-19T18:09:06Z

+export const ESTermsWithAggregationRT: rt.Type<SnapshotTermsWithAggregationForAlerts> =
+  rt.recursion('SnapshotModelRT', () =>
    rt.type({
      terms: rt.type({ field: rt.string }),
      aggregations: MetricsUIAggregationRT,
    })
-);
+  );


Suggestion: I think we can simplify this and even remove a few things

export const ESTermsWithAggregationRT = rt.type({ terms: rt.type({ field: rt.string }), aggregations: rt.UnknownRecord, });

We only need the ESTermsWithAggregationRT type to validate one thing in the is_rate function.

is_rate just needs to know whether the object is a terms agg and I think this can be inferred by whether the object has terms and aggregations attributes. So I'd say that it doesn't need to deeply validate the shape of aggregations.

Once that's changed, these could be removed, leaving only those types that is_rate is using.

ESPercentileAggRT

ESBucketScriptAggRT

ESCumulativeSumAggRT

ESCaridnalityAggRT

ESTopMetricsAggRT

ESMaxPeriodFilterExistsAggRT

We could even remove ESAggregationRT and MetricsUIAggregationRT altogether because there will be nowhere else using them. Wdyt?

It gives us the possibility to get rid of a lot of the old aggregation types which is good. As this is not the only validation there and we also check for ESSumBucketAggRT it should be fine to only validate that we have term and aggregations attributes. I applied the suggestion 👍

Nice! I think now we can safely remove these, can't we?

ESPercentileAggRT ESBucketScriptAggRT ESCumulativeSumAggRT ESCaridnalityAggRT ESTopMetricsAggRT ESMaxPeriodFilterExistsAggRT

Yes, I removed all of the types that are no longer used 👍

crespocarlos · 2024-08-19T18:25:12Z


 export const isInterfaceRateAgg = (metric: MetricsUIAggregation | undefined) => {
-  if (!MetricsUIAggregationRT.is(metric)) {
+  if (metric === undefined) {


Maybe?

Suggested change

if (metric === undefined) {

if (!metric) {

crespocarlos · 2024-08-19T18:25:29Z


 export const isMetricRate = (metric: MetricsUIAggregation | undefined): boolean => {
-  if (!MetricsUIAggregationRT.is(metric)) {
+  if (metric === undefined) {


Maybe?

Suggested change

if (metric === undefined) {

if (!metric) {

…stypesaggregationsaggregate' of https://github.com/jennypavlova/kibana into 190311-infra-replace-metricsuiaggregation-in-favor-of-estypesaggregationsaggregate

…-favor-of-estypesaggregationsaggregate

kibana-ci · 2024-08-20T13:31:41Z

💚 Build Succeeded

Buildkite Build
Commit: 6093b35
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-190495-6093b355a86a

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`infra`	1576	1575	-1
`metricsDataAccess`	288	317	+29
total			+28

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`	109	128	+19

Async chunks

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

id	before	after	diff
`infra`	1.5MB	1.5MB	+101.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
`metricsDataAccess`	6	5	-1

Page load bundle

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

id	before	after	diff
`infra`	98.8KB	97.7KB	-1.1KB
`metricsDataAccess`	57.9KB	60.2KB	+2.2KB
total			+1.1KB

Unknown metric groups

API count

id	before	after	diff
`metricsDataAccess`	109	128	+19

History

💔 Build #228523 failed 5312cc7
💔 Build #228353 failed ac7c51f
💔 Build #228352 failed ac7c51f
💔 Build #227673 failed dd6fad0
💔 Build #227577 failed b6a70068f91e4313cfd84d8fa5b187752ba1aae5

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

cc @jennypavlova

crespocarlos

LGTM 💯 ! Thanks for this PR.

kdelemme

ux-logs-management code change looks good to me!

jennypavlova added 2 commits August 14, 2024 12:27

Remove iots types and use es types

4795134

Replace MetricsUIAggregationRT

3bdc2a6

jennypavlova added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. labels Aug 14, 2024

jennypavlova self-assigned this Aug 14, 2024

Types fixes

97610bd

jennypavlova force-pushed the 190311-infra-replace-metricsuiaggregation-in-favor-of-estypesaggregationsaggregate branch from 6467553 to 97610bd Compare August 14, 2024 14:07

Merge branch 'main' into 190311-infra-replace-metricsuiaggregation-in…

3da6c68

…-favor-of-estypesaggregationsaggregate

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

dd6fad0

…-fix'

crespocarlos reviewed Aug 15, 2024

View reviewed changes

jennypavlova added 6 commits August 19, 2024 15:55

Cleanup and CR comments

f9ac2ef

Remove unused route and CR comments

8d30538

Merge branch 'main' of github.com:elastic/kibana into 190311-infra-re…

e3605ad

…place-metricsuiaggregation-in-favor-of-estypesaggregationsaggregate

Export types

8281fc8

Remove helpers and revert alerts changes

49663ce

Merge branch 'main' into 190311-infra-replace-metricsuiaggregation-in…

ac7c51f

…-favor-of-estypesaggregationsaggregate

jennypavlova marked this pull request as ready for review August 19, 2024 17:22

jennypavlova requested a review from a team as a code owner August 19, 2024 17:22

jennypavlova requested review from a team August 19, 2024 17:22

jennypavlova requested a review from a team as a code owner August 19, 2024 17:22

jennypavlova requested a review from crespocarlos August 19, 2024 17:22

botelastic Bot added the ci:project-deploy-observability Create an Observability project label Aug 19, 2024

crespocarlos reviewed Aug 19, 2024

View reviewed changes

jennypavlova added 7 commits August 20, 2024 13:00

Fix imports, CR changes

e79f2e6

Merge branch '190311-infra-replace-metricsuiaggregation-in-favor-of-e…

e31e3a8

…stypesaggregationsaggregate' of https://github.com/jennypavlova/kibana into 190311-infra-replace-metricsuiaggregation-in-favor-of-estypesaggregationsaggregate

Merge branch 'main' into 190311-infra-replace-metricsuiaggregation-in…

cbfaf05

…-favor-of-estypesaggregationsaggregate

CR change

5312cc7

Fix imports

5d70525

Merge branch 'main' into 190311-infra-replace-metricsuiaggregation-in…

30cf479

…-favor-of-estypesaggregationsaggregate

Remove no longer needed types

6093b35

jennypavlova requested a review from crespocarlos August 20, 2024 14:12

crespocarlos approved these changes Aug 21, 2024

View reviewed changes

kdelemme approved these changes Aug 21, 2024

View reviewed changes

jennypavlova merged commit 22031df into elastic:main Aug 21, 2024

kibanamachine added the v8.16.0 label Aug 21, 2024

miloszmarcinkowski mentioned this pull request Sep 4, 2024

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

Merged

1 task


		export type MetricsAPISeries = rt.TypeOf<typeof MetricsAPISeriesRT>;

		export type MetricsAPIRequest = rt.TypeOf<typeof MetricsAPIRequestRT>;

Conversation

jennypavlova commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

obltmachine commented Aug 14, 2024

🤖 GitHub comments

Uh oh!

jennypavlova commented Aug 14, 2024

Uh oh!

jennypavlova commented Aug 14, 2024

Uh oh!

jennypavlova commented Aug 14, 2024

Uh oh!

crespocarlos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Aug 19, 2024

Uh oh!

crespocarlos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jennypavlova commented Aug 14, 2024 •

edited

Loading

kibana-ci commented Aug 20, 2024 •

edited

Loading