Skip to content
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

[Infra] Change host count KPI query #188950

Merged

Conversation

jennypavlova
Copy link
Member

@jennypavlova jennypavlova commented Jul 23, 2024

Closes #188757

Summary

This PR adds an endpoint to get the hosts (monitored by the system integration) count. Currently, it supports only hosts but it can be extended to other asset types (if/when needed). So the endpoint ( POST /api/infra/{assetType}/count ) supports only 'host' as {assetType}.

⚠️ This PR adds only the endpoint - it is not used yet! To avoid having different host counts and results shown on the UI this PR is not updating the hook responsible for the request because currently the hosts shown in the table are not filtered by the system integration (showing the filtered result of this endpoint can lead to inconsistencies between the count and the results shown in the table) Once #188756 and #188752 are done we can use this endpoint.

Testing

It can't be tested in the UI so we can test it:

Using curl:
curl --location -u elastic:changeme 'http://0.0.0.0:5601/ftw/api/infra/host/count' \
--header 'kbn-xsrf: xxxx' \
--header 'Content-Type: application/json' \
--data '{
   "query": {
      "bool": {
         "must": [],
         "filter": [],
         "should": [],
         "must_not": []
      }
   },
   "from": "2024-07-23T11:34:11.640Z",
   "to": "2024-07-23T11:49:11.640Z",
   "sourceId": "default"
}'

In case of testing with oblt replace the elastic:changeme with your user and password

@jennypavlova jennypavlova added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Jul 23, 2024
@jennypavlova jennypavlova self-assigned this Jul 23, 2024
@obltmachine
Copy link

🤖 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 jennypavlova added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Jul 24, 2024
@jennypavlova jennypavlova marked this pull request as ready for review July 24, 2024 11:41
@jennypavlova jennypavlova requested review from a team as code owners July 24, 2024 11:41
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this change Jenny. I had a quick look but haven't had the chance to test it yet.

Comment on lines 51 to 60
{
term: {
'event.module': 'system',
},
},
{
term: {
'metricset.module': 'system',
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use termQuery helper functions. Also, how about creating constants for the field names?

Suggested change
{
term: {
'event.module': 'system',
},
},
{
term: {
'metricset.module': 'system',
},
},
...termQuery(EVENT_MODULE, 'system')
...termQuery(METRICSET_MODULE, 'system'),

...queryBool,
filter: [
...queryFilter,
...rangeQuery,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use the rangeQuery helper function here.

Suggested change
...rangeQuery,
...rangeQuery(new Date(from).getTime(), new Date(to).getTime())


framework.registerRoute(
{
method: 'post',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we this could be a GET request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to pass the query, filters, time range, etc. so we can even expand more the body if it is a POST. I am not sure how much will those params extend in the future so I was thinking that having the consistency with the other endpoints with similar functions can help here. But I don't have a strong opinion on that. Do you think it's better to have query params instead of the request body and convert to GET?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GET operation is preferable in this case, since nothing is being created. Infra uses POST for historical reasons, but most of them should be GET instead. The complicated part is on how to pass the filters in the query params.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed I created an issue to investigate the option to use GET instead of POST in our APIs #189170 There are some benefits and concerns described there and it would be nice also to keep them consistent and have GET requests where possible ( I totally agree that it will be better but we still need to check if the url limitations affect us) So this change won't be part of this PR

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Jul 24, 2024
Comment on lines 9 to 10
export const EVENT_MODULE = 'event.module';
export const METRICSET_MODULE = 'metricset.module';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps these could be in the constants in common instead. It will allow the client to also access them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks ✅

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT! Thanks for this change.

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jul 25, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1576 1577 +1

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 +540.0B

History

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

cc @jennypavlova

@jennypavlova jennypavlova merged commit fd8b7f6 into elastic:main Jul 25, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infra] Change host count KPI query
6 participants