[APM] Create infraMetricsClient #141735

MiriamAparicio · 2022-09-26T08:14:57Z

MiriamAparicio · 2022-09-26T08:32:37Z

x-pack/plugins/apm/server/routes/infrastructure/get_host_names.ts

  start: number;
  end: number;
 }) => {
-  const response = await esClient.search<unknown, { hostNames: Aggs }>({
-    index: [index],
+  const response = await infraMetricsClient.search<


The type of the response failed because of the type of response from the infraMetricsClient

sorenlouv · 2022-09-26T09:53:42Z

x-pack/plugins/apm/server/lib/helpers/setup_request.ts

+      infraMetricsClient: await createInfraMetricsClient({
+        infraPlugin: plugins.infra,
+        context,
+      }),


I understand why you added it here but the idea is to split up setupRequest in separate parts, so we don't have to initialise every client when we just need one of them.

So please remove it from here and invoke it separately

Yes, I completely forgot about that issue, it's almost a year old, should we prioritize it and added to 8.6?
I will add a different setupRequest for the infraMetricsClient for now

I'd love to see it split up. Your are welcome to take a stab at it after the test plan is done.

sorenlouv · 2022-09-26T10:01:22Z

...rver/lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client.ts

+  context: ApmPluginRequestHandlerContext;
+}) {
+  return {
+    search: async (opts: ESSearchRequest) => {


index is required by default, so you need to remove it:

Suggested change

search: async (opts: ESSearchRequest) => {

search: async <TParams extends Omit<ESSearchRequest, 'index'>>(

opts: TParams

@dgieselaar What else is needed to ensure that aggregations are inferred correctly? Do we need to set a manual return type like InferSearchResponseOf<TDocument, TParams> ?

I added the params type, but I still have the same errors, my errors are in the response type from the client

sorenlouv · 2022-09-27T09:04:24Z

x-pack/plugins/apm/server/lib/helpers/setup_infra_metrics_request.ts

+      }),
+    };
+  });
+}


Why do we need this function? Why not call createInfraMetricsClient directly?

sorenlouv · 2022-09-27T09:05:59Z

x-pack/plugins/apm/server/routes/infrastructure/route.ts

@@ -29,12 +30,9 @@ const infrastructureRoute = createApmServerRoute({
    podNames: string[];
  }> => {
    const setup = await setupRequest(resources);
+    const setupInfraMetrics = await setupInfraMetricsRequest(resources);


Why are we not calling createInfraMetricsClient directly?

Suggested change

const setupInfraMetrics = await setupInfraMetricsRequest(resources);

const infraMetricsClient = await createInfraMetricsClient(resources);

sorenlouv · 2022-09-27T09:08:34Z

x-pack/plugins/apm/server/routes/services/get_service_instance_container_metadata.ts

+  if (!infraMetricsClient) {
    return undefined;
  }


Is this check necessary? Isn't infraMetricsClient required?

No, it was checking for the index before, but if the client is created we don't need to check it

…s-client

sorenlouv · 2022-09-27T12:46:41Z

...rver/lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client.ts

+        ...opts,
+      };
+
+      return esClient.asCurrentUser.search(searchParams);


Try this (credits goes to @dgieselaar)

Suggested change

return esClient.asCurrentUser.search(searchParams);

const res = esClient.asCurrentUser.search(

searchParams

) as unknown as Promise<{

body: any;

}>;

return unwrapEsResponse(res);

sorenlouv · 2022-09-27T12:47:10Z

...rver/lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client.ts

+    search: async <TParams extends Omit<ESSearchRequest, 'index'>>(
+      opts: TParams
+    ) => {


Suggested change

search: async <TParams extends Omit<ESSearchRequest, 'index'>>(

opts: TParams

) => {

async search<TDocument, TParams extends InfraMetricsSearchParams>(

opts: TParams

): Promise<InferSearchResponseOf<TDocument, TParams>> {

Where InfraMetricsSearchParams is:

type InfraMetricsSearchParams = Omit<ESSearchRequest, 'index'> & { body: { size: number; track_total_hits: boolean | number; }; };

InfraMetricsSearchParams is needed to make size and track_total_hits required.

sorenlouv · 2022-09-27T12:47:41Z

x-pack/plugins/apm/server/routes/services/get_service_overview_container_metadata.ts

+  const response = await infraMetricsClient.search<
+    unknown,
+    ResponseAggregations
+  >({


This shouldn't be needed

Suggested change

const response = await infraMetricsClient.search<

unknown,

ResponseAggregations

>({

const response = await infraMetricsClient.search({

sorenlouv · 2022-09-27T12:50:15Z

x-pack/plugins/apm/server/routes/services/get_service_overview_container_metadata.ts

    _source: [KUBERNETES, CONTAINER],
    size: 0,


nit: We are not using source (size: 0) so there's no need to specify _source: [KUBERNETES, CONTAINER].

sorenlouv · 2022-09-27T18:49:41Z

x-pack/plugins/apm/server/routes/infrastructure/route.ts

-
-    const {
+    const { plugins, context } = resources;
+    const infraMetricsClient = await createInfraMetricsClient({


Do we need to await createInfraMetricsClient?

Suggested change

const infraMetricsClient = await createInfraMetricsClient({

const infraMetricsClient = createInfraMetricsClient({

sorenlouv · 2022-09-27T18:50:23Z

...rver/lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client.ts

+
+export type InfraClient = Awaited<ReturnType<typeof createInfraMetricsClient>>;
+
+export async function createInfraMetricsClient({


Shouldn't be async afaict

Suggested change

export async function createInfraMetricsClient({

export function createInfraMetricsClient({

sorenlouv · 2022-09-27T18:56:06Z

x-pack/plugins/apm/server/routes/services/get_service_overview_container_metadata.ts

@@ -19,6 +17,7 @@ import {
  KUBERNETES_REPLICASET_NAME,
  KUBERNETES_DEPLOYMENT_NAME,
 } from '../../../common/elasticsearch_fieldnames';
+import { InfraClient } from '../../lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client';

 type ServiceOverviewContainerMetadataDetails =
  | {


This should be inferred from the query and not explicitly typed

sorenlouv · 2022-09-27T18:56:45Z

x-pack/plugins/apm/server/routes/services/get_service_overview_container_metadata.ts

@@ -19,6 +17,7 @@ import {
  KUBERNETES_REPLICASET_NAME,
  KUBERNETES_DEPLOYMENT_NAME,
 } from '../../../common/elasticsearch_fieldnames';
+import { InfraClient } from '../../lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client';


Can you please rename this.

Suggested change

import { InfraClient } from '../../lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client';

import { InfraMetricsClient } from '../../lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client';

elasticmachine · 2022-09-28T14:23:31Z

Pinging @elastic/apm-ui (Team:APM)

sorenlouv · 2022-09-28T20:43:44Z

x-pack/plugins/apm/server/routes/services/route.ts

+    const { plugins, context } = resources;
+    const infraMetricsClient = await createInfraMetricsClient({
+      infraPlugin: plugins.infra,
+      context,
+    });


I think we can simplify this a bit if we just pass resources as arg:

Suggested change

const { plugins, context } = resources;

const infraMetricsClient = await createInfraMetricsClient({

infraPlugin: plugins.infra,

context,

});

const infraMetricsClient = await createInfraMetricsClient(resources);

I leave like this, infraPlugin is not part of the resources, so I will need to extracted somewhere else

sorenlouv

lgtm. Thanks @MiriamAparicio !

kibana-ci · 2022-09-29T10:22:24Z

💚 Build Succeeded

Buildkite Build
Commit: 563910f

Metrics [docs]

✅ unchanged

History

💚 Build #76756 succeeded bd3de94ba03db2f628aa247836848f856a2d298a
💔 Build #76494 failed 40f20d5f63dcb312b9f98e4366b08483ea1d446f
💔 Build #76412 failed a995b79
💔 Build #76393 failed dc6b4f3
💚 Build #76130 succeeded 072ab4e
💔 Build #75940 failed 7ee3c20

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

...rver/lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client.ts

Create infraMetricsClient

8bad190

MiriamAparicio added the v8.6.0 label Sep 26, 2022

MiriamAparicio commented Sep 26, 2022

View reviewed changes

sorenlouv reviewed Sep 26, 2022

View reviewed changes

remove infraclient from setup request and create its own

a4488b2

MiriamAparicio changed the title ~~Create infraMetricsClient~~ [APM] Create infraMetricsClient Sep 27, 2022

sorenlouv reviewed Sep 27, 2022

View reviewed changes

MiriamAparicio added 2 commits September 27, 2022 10:30

Merge remote-tracking branch 'upstream/main' into 140924-infra-metric…

720264d

…s-client

Remove the setupInfraMetrics, use createInfraMetricsClient

7ee3c20

sorenlouv reviewed Sep 27, 2022

View reviewed changes

fix types

072ab4e

sorenlouv reviewed Sep 27, 2022

View reviewed changes

MiriamAparicio and others added 3 commits September 28, 2022 11:12

typing createInfraMetrics client

dc6b4f3

Fix types and simplify get_host_names

a995b79

fix get_service_overview_container_metadata types

7882712

MiriamAparicio added Team:APM - DEPRECATED Use Team:obs-ux-infra_services. release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Sep 28, 2022

MiriamAparicio marked this pull request as ready for review September 28, 2022 14:23

MiriamAparicio requested a review from a team as a code owner September 28, 2022 14:23

sorenlouv reviewed Sep 28, 2022

View reviewed changes

sorenlouv approved these changes Sep 28, 2022

View reviewed changes

pass resources as a param instead

f942e61

MiriamAparicio force-pushed the 140924-infra-metrics-client branch from bd3de94 to f942e61 Compare September 29, 2022 09:19

Merge branch 'main' into 140924-infra-metrics-client

563910f

MiriamAparicio merged commit 4bab191 into elastic:main Sep 29, 2022

MiriamAparicio deleted the 140924-infra-metrics-client branch September 29, 2022 10:23

sorenlouv reviewed Sep 29, 2022

View reviewed changes

...rver/lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client.ts Show resolved Hide resolved

MiriamAparicio mentioned this pull request Oct 4, 2022

[APM] Added required fields to infra metrics client #142569

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Create infraMetricsClient #141735

[APM] Create infraMetricsClient #141735

MiriamAparicio commented Sep 26, 2022

MiriamAparicio Sep 26, 2022

sorenlouv Sep 26, 2022 •

edited

Loading

MiriamAparicio Sep 26, 2022

sorenlouv Sep 26, 2022

sorenlouv Sep 26, 2022

sorenlouv Sep 26, 2022

MiriamAparicio Sep 26, 2022 •

edited

Loading

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 27, 2022

MiriamAparicio Sep 27, 2022

sorenlouv Sep 27, 2022

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 27, 2022

sorenlouv Sep 27, 2022

sorenlouv Sep 27, 2022

sorenlouv Sep 27, 2022

sorenlouv Sep 27, 2022

sorenlouv Sep 27, 2022

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 27, 2022 •

edited

Loading

elasticmachine commented Sep 28, 2022

sorenlouv Sep 28, 2022 •

edited

Loading

MiriamAparicio Sep 29, 2022

sorenlouv left a comment

kibana-ci commented Sep 29, 2022

	search: async (opts: ESSearchRequest) => {
	search: async <TParams extends Omit<ESSearchRequest, 'index'>>(
	opts: TParams

	const setupInfraMetrics = await setupInfraMetricsRequest(resources);
	const infraMetricsClient = await createInfraMetricsClient(resources);

-      return esClient.asCurrentUser.search(searchParams);
+      const res = esClient.asCurrentUser.search(
+        searchParams
+      ) as unknown as Promise<{
+        body: any;
+      }>;
+      return unwrapEsResponse(res);

	const infraMetricsClient = await createInfraMetricsClient({
	const infraMetricsClient = createInfraMetricsClient({


		export type InfraClient = Awaited<ReturnType<typeof createInfraMetricsClient>>;

		export async function createInfraMetricsClient({

	export async function createInfraMetricsClient({
	export function createInfraMetricsClient({

	import { InfraClient } from '../../lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client';
	import { InfraMetricsClient } from '../../lib/helpers/create_es_client/create_infra_metrics_client/create_infra_metrics_client';

[APM] Create infraMetricsClient #141735

[APM] Create infraMetricsClient #141735

Conversation

MiriamAparicio commented Sep 26, 2022

Choose a reason for hiding this comment

sorenlouv Sep 26, 2022 • edited Loading

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

MiriamAparicio Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

sorenlouv Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

sorenlouv Sep 27, 2022 • edited Loading

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

sorenlouv Sep 27, 2022 • edited Loading

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

sorenlouv Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

sorenlouv Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Sep 28, 2022

sorenlouv Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv left a comment

Choose a reason for hiding this comment

kibana-ci commented Sep 29, 2022

💚 Build Succeeded

Metrics [docs]

History

sorenlouv Sep 26, 2022 •

edited

Loading

MiriamAparicio Sep 26, 2022 •

edited

Loading

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 27, 2022 •

edited

Loading

sorenlouv Sep 28, 2022 •

edited

Loading