-
Notifications
You must be signed in to change notification settings - Fork 422
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
Adding HTTP metrics collection to Kibana integration #6169
Conversation
🌐 Coverage report
|
@@ -8,7 +8,7 @@ If the Kibana instance is using a basepath in its URL, you must set the `basepat | |||
|
|||
## Compatibility | |||
|
|||
The `kibana` package works with Kibana 8.5.0 and later. | |||
The `kibana` package works with Kibana 8.8.0 and later. |
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 assume this change is because of the new dataset? All the other dataset still work for the older versions?
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.
If the compatibility version in the manifest has changed then you cannot install this (and later) versions of the package on that cluster.
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.
Yes, this change is because the background task HTTP API endpoint is only available in Kibana 8.8.0+ but all other datasets should still work for older versions.
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.
Actually realized that the Kibana PR was merged for 8.9 not 8.8 so updated again for 8.9
"version": "20.04.6 LTS (Focal Fossa)" | ||
} | ||
}, | ||
"http": { |
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 suggest to put this under kibana.background_task_utilization
instead.
@jsoriano @joshdover As we have dataset now, I wonder if we still need this prefixing of metrics? Where should we be heading 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.
ECS recommends actually using a prefix to avoid conflicts with future additions to the common schema: https://www.elastic.co/guide/en/ecs/current/ecs-custom-fields-in-ecs.html#_proper_names
Since this is a fairly specific metric that I don't see getting added to ECS, I think a prefix might make sense 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'm not sure I understand what the recommendation is @joshdover, but to chip in, for the other data streams we put them under {product}.{data_stream}.{metric}
so I'd prefer to stick to that unless that would cause problems.
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 think we all made the same recommendation only with different words?
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 agree that kibana.background_task_utilization
makes sense! Looking at the metricbeat HTTP docs, I only see an option for specifying namespace
which seems to automatically add it under the http
field (http.${namespace}
. Would I need to specify an ingest pipeline to rename it?
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.
You can either do it with a rename in the agent or in the ingest pipline. Ideally, you could just specific the prefix :-( Worth adding a beats issue to get this fixed long term?
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 added the rename in the agent and opened an issue in beats: elastic/beats#35443
show_user: true | ||
default: | ||
- http://localhost:5601 | ||
- name: username |
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.
Should we recommend usage of API keys?
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.
Maybe...
But, the Metricbeat module that powers the other data streams here do not support using API keys (see elastic/beats#29271), which might lead to some confusion?
Ideally we would prioritize adding API key support for the Metricbeat modules and then add it into each Integration as well.
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.
cc @smith
@miltonhultgren Would be great to get your review on this one. I'm also curious to hear your thoughts on the grouping. The |
I'll have a look but I wanted to clarify (as usual) that this is meant only for Agent based collection? @ruflin The grouping makes sense in the way that it's separate from the things that power the Stack Monitoring UI. You could add more data streams to the same type though I'm not sure if the label for that group should be the name of the "module" or "technical interface" (the Stack Monitoring stuff also hits the HTTP API for metrics). In other places I think we just call it "Metrics". The main point is that we're free to name this new group anything we want. |
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.
Overall this LGTM!
I had some questions/concerns about the data stream naming and the field prefixes and some nitpicks on the copytext.
packages/kibana/changelog.yml
Outdated
@@ -1,4 +1,9 @@ | |||
# newer versions go on top | |||
- version: "2.3.5" | |||
changes: | |||
- description: Add HTTP metrics to collect background task utilization metric |
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.
NIT:
- description: Add HTTP metrics to collect background task utilization metric | |
- description: Add background task utilization metric |
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.
Updated in 051981f
packages/kibana/docs/README.md
Outdated
|
||
### Background task utilization | ||
|
||
Background task utilization data stream uses the `/api/task_manager/_background_task_utilization` API of Kibana, which is available starting in 8.8. |
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.
NIT:
Background task utilization data stream uses the `/api/task_manager/_background_task_utilization` API of Kibana, which is available starting in 8.8. | |
The Background task utilization data stream uses the `/api/task_manager/_background_task_utilization` API of Kibana, which is available starting in 8.8. |
or
Background task utilization data stream uses the `/api/task_manager/_background_task_utilization` API of Kibana, which is available starting in 8.8. | |
This data stream uses the `/api/task_manager/_background_task_utilization` API of Kibana, which is available starting in 8.8. |
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.
Updated in 051981f
| timestamp | | alias | | ||
|
||
|
||
An example event for `background_task_utilization` looks as following: |
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.
NIT:
An example event for `background_task_utilization` looks as following: | |
An example event for `background_task_utilization` looks as follows: |
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.
This document is auto-generated using the event placeholder {{event "background_task_utilization"}}
in packages/kibana/_dev/build/docs/README.md
so I can't change this wording without changing the auto-generation, which will likely change all the docs files in this repo 🙈
type: metrics | ||
title: Kibana background task utilization metrics | ||
release: beta | ||
dataset: kibana.http_metrics.background_task_utilization |
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.
dataset: kibana.http_metrics.background_task_utilization | |
dataset: kibana.background_task_utilization |
I don't think it's needed to add the type into the dataset name. We've added that to the Stack Monitoring legacy streams to not block the usage of that name for when/if new data streams are introduced for the "same" data but natively to the Integration.
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.
Updated in 051981f
title: Kibana background task utilization metrics | ||
description: Collect Kibana background task utilization metrics |
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.
It would be nice if we could include some information about what those metrics are and what they are useful for.
NIT:
Maybe we don't need to write Kibana everywhere since the context should be clear to the user from being on the Kibana integration page?
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.
Updated in 051981f
"version": "20.04.6 LTS (Focal Fossa)" | ||
} | ||
}, | ||
"http": { |
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'm not sure I understand what the recommendation is @joshdover, but to chip in, for the other data streams we put them under {product}.{data_stream}.{metric}
so I'd prefer to stick to that unless that would cause problems.
@miltonhultgren Yes, this is only meant for agent-based collection. We did not update the Kibana module in metricbeat. |
@miltonhultgren I had to update the fields mappings for some of the other datastreams for 8.9 to get the tests to pass: e8d1489 Is this the right way to do that? |
@ymao1 Yes, the Integration system tests asserts that all fields in the ingested document have a mapping (which is good/bad) so that's the right way forward 👍🏼 |
Package kibana - 2.3.5 containing this change is available at https://epr.elastic.co/search?package=kibana |
What does this PR do?
Updates the Kibana integration to use the metricbeat HTTP module to collect background task utilization metrics from the Kibana API endpoint added in this PR.
Checklist
changelog.yml
file.Author's Checklist
How to test this PR locally
elastic-package
:elastic-package stack up --version=8.9.0-SNAPSHOT -v -d
cd integrations/packages/kibana
elastic-package build
elastic-package stack up -v -d --services package-registry
HTTP metrics background task utilization
integrationhttps://kibana:5601
and username/passwordelastic:changeme
metrics-kibana.background_task_utilization-default
GET metrics-kibana.background_task_utilization-default/_search
) with thekibana.background_task_utilization.stats.value.load
field populatedRelated issues
Screenshots