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

[Docker] Add dimensions to metrics data streams, except for events #6390

Merged
merged 13 commits into from
Jun 5, 2023

Conversation

constanca-m
Copy link
Contributor

@constanca-m constanca-m commented May 31, 2023

What does this PR do?

Add dimensions to metrics data streams, except for events.

Details

For all data streams, there exists these set of dimensions:

- external: ecs
  name: cloud.account.id
  dimension: true
- external: ecs
  name: cloud.provider
  dimension: true
- external: ecs
  name: cloud.region
  dimension: true
- external: ecs
  name: cloud.availability_zone
  dimension: true
- external: ecs
  name: cloud.instance.id
  dimension: true

In case it is running on cloud.More info on this can be found in #5193 (comment).

The dimensions set for each data stream are container.id and service.address, since the ID is unique per Docker daemon. Some data streams did not hold value for container.id, so a similar field was selected (see below).

Container: No change in the number of documents with TSDB disabled v enabled
image

CPU: No change in the number of documents with TSDB disabled v enabled
image

Diskio: No change in the number of documents with TSDB disabled v enabled
image

Event: Does not have any metrics

Healthcheck: No change in the number of documents with TSDB disabled v enabled
image

Image: No change in the number of documents with TSDB disabled v enabled. Instead of container.id, we use docker.image.id.current.

image

Also changed:

image

To:

image

Info: docker.info.id instead of container.id is used (last one holds no value).
image

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • No change in the number of documents after enabling TSDB

How to test this PR locally

  1. Clone this branch.
  2. Update the service registry to include this version of Docker and enable TSDB.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
@constanca-m constanca-m requested a review from a team as a code owner May 31, 2023 10:24
@constanca-m constanca-m self-assigned this May 31, 2023
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
@elasticmachine
Copy link

elasticmachine commented May 31, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-06-05T13:47:31.128+0000

  • Duration: 20 min 51 sec

Test stats 🧪

Test Results
Failed 0
Passed 38
Skipped 0
Total 38

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented May 31, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (0/0) 💚
Files 100.0% (0/0) 💚 3.846
Classes 100.0% (0/0) 💚 3.846
Methods 96.667% (29/30) 👍 8.899
Lines 100.0% (0/0) 💚 13.666
Conditionals 100.0% (0/0) 💚

constanca-m and others added 3 commits June 1, 2023 10:11
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
@constanca-m
Copy link
Contributor Author

Hey @gizas , I added agent.id (dimension as well) to all metrics data streams. Reasoning can be found in the thread following this comment #5193 (comment)

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Copy link
Contributor

@tetianakravchenko tetianakravchenko left a comment

Choose a reason for hiding this comment

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

to align with #5193 (comment) I think that host.name should be added. I see that cloud.* are not defined in docker package, so lets skip it.

@@ -27,6 +27,7 @@
Total number of existing containers.
- name: id
type: keyword
dimension: true
Copy link
Contributor

Choose a reason for hiding this comment

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

this data_stream does not contain container.id or it is just missing in fields definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, container.id has no value for this data stream

@@ -2,6 +2,7 @@
name: ecs.version
- external: ecs
name: service.address
dimension: true
- external: ecs
name: service.type
- external: ecs
Copy link
Contributor

Choose a reason for hiding this comment

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

container.id for image data_stream is it not set as a dimension on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @tetianakravchenko The field doesn't hold any value for that data stream, that's why docker.image.id.current is a dimension.

@constanca-m
Copy link
Contributor Author

constanca-m commented Jun 2, 2023

to align with #5193 (comment) I think that host.name should be added. I see that cloud.* are not defined in docker package, so lets skip it.

I think service.address should be enough for different hosts. What do you think @tetianakravchenko ?

@tetianakravchenko
Copy link
Contributor

tetianakravchenko commented Jun 5, 2023

to align with #5193 (comment) I think that host.name should be added. I see that cloud.* are not defined in docker package, so lets skip it.

I think service.address should be enough for different hosts. What do you think @tetianakravchenko ?

from my understanding there should be either the ip address or the socket, which might not be unique enough, if we run in different cloud providers/different cloud accounts instance with the docker service running, isn't it?

in the sample file -

"service": {
"address": "/var/run/docker.sock",
, so it might be the same for different instances, so we need to add some information about the host to separate data from different hosts

@constanca-m
Copy link
Contributor Author

from my understanding there should be either the ip address or the socket, which might not be unique enough, if we run in different cloud providers/different cloud accounts instance with the docker service running, isn't it?

in the sample file -

"service": {
"address": "/var/run/docker.sock",

, so it might be the same for different instances, so we need to add some information about the host to separate data from different hosts

You're right. In my example they were different, but now that I think of it, they were only different because I made them be. I will fix it.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
@constanca-m constanca-m merged commit e867027 into elastic:main Jun 5, 2023
@constanca-m constanca-m deleted the docker-add-dimensions branch June 5, 2023 16:15
@elasticmachine
Copy link

Package docker - 2.5.1 containing this change is available at https://epr.elastic.co/search?package=docker

sodhikirti07 pushed a commit that referenced this pull request Jun 15, 2023
…6390)

* Add dimensions to container.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Add dimensions to cpu.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Add dimensions to diskio and healthcheck.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Add dimensions to info, memory and network.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Add dimensions to image data stream.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Update version.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Update changelog.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Update version.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Add agent.id as dimension

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Update readme.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Add host.name as dimension.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

* Update dimensions.

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>

---------

Signed-off-by: constanca-m <constanca.manteigas@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants