[Infra] Create service in apm_data_access to fetch APM colleced hosts
#189046
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| import { estypes } from '@elastic/elasticsearch'; | ||
| import { rangeQuery } from '@kbn/observability-plugin/server'; | ||
| import { RegisterParams } from '../register_services'; | ||
|
|
||
| export interface ServicesHostNamesParams { | ||
| query: estypes.QueryDslQueryContainer; | ||
| from: number; | ||
| to: number; | ||
| limit: number; | ||
| } | ||
|
|
||
| export function createGetHostNames(params: RegisterParams) { | ||
| return async ({ from: timeFrom, to: timeTo, query, limit }: ServicesHostNamesParams) => { | ||
| const { apmIndices, esClient } = await params.getResourcesForServices(); | ||
|
There was a problem hiding this comment. I think we should stay away from patterns like this, and expect the consumer to pass in asynchronously resolved dependencies, such as getting the APM indices. If we don't do that, we'll end up with a ton of small requests - I see this happen with e.g. the ML plugin that does a ton of small requests because it encapsulates privilege and other checks in each service call. For instance: #161229 There was a problem hiding this comment. Yeah, I was just having second thoughts about this approach. There was a problem hiding this comment. though it's kind of weird asking the consumer to pass the APM indices to a service in the There was a problem hiding this comment. it is a little weird, and there might be a better way (e.g. to have a per-request service that does per-request caching, but that also might create some risks). There was a problem hiding this comment. I can't really see the risks of getting the APM indices when calling a service in the I'm more concerned about the fact I'm using the internal user to get both apm indices and typed esClient. There was a problem hiding this comment. One downside is potentially requesting APM indices twice. There was a problem hiding this comment.
This is my (only) concern. It might end up being more than twice however. Especially in the context of e.g. rule executors this becomes more important. My experience is that if we abstract away async operations, people lose awareness of what is happening as part of the service call, and it leads to performance bottlenecks. I'd be less concerned about this if we have really good insight in production bottlenecks, but we don't. There was a problem hiding this comment. For the lack of better alternatives, I'll leave it up to the consumer the send the APM indices for now. It's bad DX but for the time being it's better than end up in situation where it might lead to performance problems. |
||
|
|
||
| const esResponse = await esClient.search({ | ||
| index: [apmIndices.metric, apmIndices.transaction, apmIndices.span, apmIndices.error], | ||
|
There was a problem hiding this comment. This will be slow, because you're aggregating over all data. Even if it's just a single service, you'll run into performance issues. What would be more efficient here is if you figure out what data source to query first (tx metrics or raw events, 1m vs 10m and 60m), and then execute it. There was a problem hiding this comment. that would be the same thing that the There was a problem hiding this comment. yeah, how involved would that be? There was a problem hiding this comment. it's a relatively big change. A few things would have to be moved (could be more):
There was a problem hiding this comment. ok, sounds fine to do it in a follow-up but I'd like to make it happen, to set a good first example There was a problem hiding this comment. yeah, makes sense. I'm going to get a sense of how much effort it would be. it might as well be doable in this PR. |
||
| track_total_hits: false, | ||
| ignore_unavailable: true, | ||
| size: 0, | ||
| query: { | ||
| bool: { | ||
| filter: [query, ...rangeQuery(timeFrom, timeTo)], | ||
| }, | ||
| }, | ||
| aggs: { | ||
| hostNames: { | ||
| terms: { | ||
| field: 'host.name', | ||
|
There was a problem hiding this comment. there is a constant for this somewhere ( There was a problem hiding this comment. not in the There was a problem hiding this comment. yeah, let's move the constants then There was a problem hiding this comment. I'll do that in a follow up PR, over 250 files will be changed after this There was a problem hiding this comment. package might be better at that point, WDYT? There was a problem hiding this comment. yeah. When doing this #189046 (comment), some types could also be moved to this new package. |
||
| size: limit, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| return esResponse.aggregations?.hostNames.buckets.map((bucket) => bucket.key as string) ?? []; | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| import { APMIndices } from '..'; | ||
| import { getTypedSearch } from '../../utils/create_typed_es_client'; | ||
| import { createGetHostNames } from './get_host_names'; | ||
|
|
||
| interface Resources { | ||
| apmIndices: APMIndices; | ||
| esClient: ReturnType<typeof getTypedSearch>; | ||
| } | ||
| export interface RegisterParams { | ||
| getResourcesForServices: () => Promise<Resources>; | ||
| } | ||
|
|
||
| export function registerServices(params: RegisterParams) { | ||
| return { | ||
| getHostNames: createGetHostNames(params), | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License | ||
| * 2.0; you may not use this file except in compliance with the Elastic License | ||
| * 2.0. | ||
| */ | ||
|
|
||
| import { ElasticsearchClient } from '@kbn/core-elasticsearch-server'; | ||
| import { ESSearchRequest, InferSearchResponseOf } from '@kbn/es-types'; | ||
|
|
||
| type RequiredParams = ESSearchRequest & { | ||
| size: number; | ||
| track_total_hits: boolean | number; | ||
| }; | ||
|
|
||
| export type TypedSearch = ReturnType<typeof getTypedSearch>; | ||
| export function getTypedSearch(esClient: ElasticsearchClient) { | ||
| async function search<TDocument, TParams extends RequiredParams>( | ||
| opts: TParams | ||
| ): Promise<InferSearchResponseOf<TDocument, TParams>> { | ||
| return esClient.search<TDocument>(opts) as Promise<any>; | ||
| } | ||
|
|
||
| return { search }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried changing the APM index patterns on the APM settings page and see if the correct indices are returned with this
createInternalRepository?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested it and it worked. APM also uses it here here. It could be due to the fact that when APM indices are changed on the Settings page, Kibana does a full page refresh.