-
Notifications
You must be signed in to change notification settings - Fork 458
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
Add containerd package and cpu,memory,blkio data streams #2522
Add containerd package and cpu,memory,blkio data streams #2522
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
|
||
- name: write.bytes | ||
type: long | ||
format: bytes |
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 units
and metric_type
be added as well?
for other fields as well - https://github.com/elastic/observability-dev/blob/fef856060729b1ff233774941711303a59a77b30/docs/dc/integrations/development/generic_guidelines.md#document-all-fields
@@ -0,0 +1,2 @@ | |||
- external: ecs | |||
name: container.id |
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 dimension: true
be set for this fields? as I see usually it is set on *.name field - https://github.com/elastic/integrations/blob/master/packages/kubernetes/data_stream/container/fields/base-fields.yml#L127-L128, but it is not possible to expose name, right?
the same for memory and cpu datastreams
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 right it is not possible. But I think that container.id is the best place.
@@ -0,0 +1,2 @@ | |||
- external: ecs |
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.
in sample event there is an ecs.version, should it be added here?
actually I thought that those fields are more or less a default for each integration - https://github.com/elastic/integrations/blob/master/packages/kubernetes/data_stream/container/fields/ecs.yml, or?
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 seems that some are added by default like the ecs.version
, service.address
and service.type
. The rest are not default. For example containerd adds container.id
while kubernetes adds orchestrator.cluster.name
|
||
- name: usage.user.ns | ||
type: double | ||
metric_type: gauge |
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.
Pinging @elastic/integrations (Team:Integrations) |
/test |
@akshay-saraswat could you take a look at the screenshot of the dashboard I have created for containerd? Unfortunately we cannot extract informations from the metrics like containers' statuses which would be something nice to see in a dashboard. |
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 looks good. Just 2 comments:
- In the visualisations
Read/Write Bytes Per Container
the Y axis has too big numbers, wondering if we could somehow curate it and make it be shown like 2M instead of 2.000.000 etc. Not sure if possible through Kibana though, maybe there is a setting in the Visualisation options. Also I see that you are using Lens Line charts. I remember in the past we prefered using TSVB charts though for many reasons. @kaiyan-sheng does it happen to have more context here? - I see the package has not tests, but you can add them with a mock server already and ensure that package is tested. See https://github.com/elastic/integrations/tree/master/packages/docker/_dev/deploy/docker as an example and a past k8s version https://github.com/elastic/integrations/tree/b189a92444c4dfc33a3462fdad21b40a5bf8ae4e/packages/kubernetes/_dev/deploy/docker
Yes we can. I had set the value type to default instead of bytes. Also I have created the same dashboard with TSVB as well. So I am good either way. |
@ChrsMark Yep @MichaelKatsoulis got it, setting the value type to bytes will give 2M instead of 2,000,000 🙂 |
@ChrsMark , @tetianakravchenko could do a final review? I decided to go with the TSVB dashboard. I also added mock tests. |
@MichaelKatsoulis did you update the screenshot after the latest dashboard changes? |
Yes the screenshot is correct , just without the |
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.
LGTM!
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.
* Add containerd package and cpu,memory,blkio data streams
What does this PR do?
After merging elastic/beats#29247 which introduced
containerd
module this PRcreates containerd package with cpu, memory and blkio data streams.
Checklist
changelog.yml
file.How to test this PR locally
elastic-package up --version=8.1.0-SNAPSHOT -v -d
kind create cluster --image 'kindest/node:v1.21.1
Docker exec
in the kind kubernetes node and configurecontainerd
to server metrics. Follow instructions here.Related issues
Screenshots