-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[APM] Added required fields to infra metrics client #142569
[APM] Added required fields to infra metrics client #142569
Conversation
Pinging @elastic/apm-ui (Team:APM) |
@@ -9,7 +9,12 @@ import { ESSearchRequest, InferSearchResponseOf } from '@kbn/es-types'; | |||
import { APMRouteHandlerResources } from '../../../../routes/typings'; | |||
import { getInfraMetricIndices } from '../../get_infra_metric_indices'; | |||
|
|||
type InfraMetricsSearchParams = Omit<ESSearchRequest, 'index'>; | |||
type InfraMetricsSearchParams = Omit<ESSearchRequest, 'index'> & { | |||
body: { |
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.
Do you really need the body
here?
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.
I was following @sqren suggestion here: #141735 (comment)
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.
No we don't, my bad! I suggested it based on what we have for the apmEventClient
:
kibana/x-pack/plugins/apm/server/lib/connections/get_connection_stats/get_stats.ts
Lines 57 to 61 in 53bf927
const response = await apmEventClient.search('get_connection_stats', { | |
apm: { | |
events: [ProcessorEvent.metric], | |
}, | |
body: { |
But the infraMetricsClient
doesn't have a body
params so we can omit that.
Thanks for adding the required fields. 💯 |
body: { | ||
size: number; | ||
track_total_hits: boolean | number; | ||
}; |
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.
body: { | |
size: number; | |
track_total_hits: boolean | number; | |
}; | |
size: number; | |
track_total_hits: boolean | number; |
cebf5e6
to
9eddf10
Compare
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: |
@sqren can you take a look at |
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.
Looks great. We don't use hits.total.value
in any of these queries so track_total_hits
can safely be disabled (false
) as you did.
* Added required fields to infra metrics client * add fields to the requests * [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs' Co-authored-by: kibanamachine <[email protected]>
* Added required fields to infra metrics client * add fields to the requests * [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs' Co-authored-by: kibanamachine <[email protected]>
Little improvement in the infra metrics client, added
size
andtrack_total_hits
as required fields.