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 Input Plugin Measure Thin Pool Minimum Free Space #6084

Merged
merged 10 commits into from
Jul 15, 2019
Merged

Docker Input Plugin Measure Thin Pool Minimum Free Space #6084

merged 10 commits into from
Jul 15, 2019

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Jul 8, 2019

Closes #5758

This adds a new measurement docker_thin_pool with the field minimum_free_space measured in bytes.

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@glinton glinton added area/docker feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Jul 8, 2019
@glinton glinton added this to the 1.12.0 milestone Jul 8, 2019
@danielnelson
Copy link
Contributor

I think we may want to reorganize the storage driver metrics. Right now the devicemapper metrics are spread out over several measurements:

- docker
  - fields:
  	- pool_blocksize

- docker_data
  - tags:
    - unit
    - engine_host
    - server_version
  - fields:
    - available
    - total
    - used

- docker_metadata
  - tags:
    - unit
    - engine_host
    - server_version
  - fields:
    - available
    - total
    - used

We can't get rid of these, since we don't want to break any queries, dashboards, alerts, etc. However, we can deprecate them and try again. I think this layout would be preferable:

- docker_devicemapper
  - tags:
    - engine_host
    - server_version
    - pool_name
  - fields:
    - data_space_available_bytes
    - data_space_total_bytes
    - data_space_used_bytes
    - metadata_space_available_bytes
    - metadata_space_total_bytes
    - metadata_space_used_bytes
    - thin_pool_minimum_free_space_bytes
    - (other stuff)

I like this layout more because it gives us room to grow, without a measurement explosion, and it groups all the devicemapper related values into a single measurement.

@GeorgeMac
Copy link
Contributor Author

GeorgeMac commented Jul 9, 2019

I like it @danielnelson

update: as I implement this I am just adding a note here that I will start with precisely the list of fields you describe there (minus "other stuff") with the addition of pool_blocksize. Since there are all byte measurements which I imagine might be the most useful. Some other possibilities are:

  • base_device_size_bytes (e.g. 10MB)
  • backing_filesystem (e.g. ext4)
  • udev_sync_supported (e.g. true)
  • data_file (e.g. /dev/loop1)
  • metadata_file (e.g. /dev/loop2)
  • data_loop_file (e.g. /var/lib/docker/devicemapper/data)
  • metadata_loop_file (e.g. /var/lib/docker/devicemapper/metadata)
  • deferred_removal_enabled (e.g. true)
  • deferred_deletion_enabled (e.g. true)
  • deferred_deleted_device_count (e.g. 1)
  • library_version (e.g. 1.02.145 (2017-11-03))

Do we want any or all of these to be included in this PR?

Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

It's hard to know where to draw the line between collecting everything and not enough, there isn't a correct answer really. My instinct is to add only base_device_size_bytes now and leave the rest off for later.

@akhon Do you need to monitor any of these other values?

plugins/inputs/docker/docker.go Outdated Show resolved Hide resolved
@@ -142,6 +142,23 @@ some storage drivers such as devicemapper.
- total
- used

The above measurements for the devicemapper storage driver can now be found in the new `docker_devicemapper` measurement

- docker_devicemapper
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to fix this, but this is good opportunity to show an obscure README issue. In Markdown, or maybe it's only Github Markdown, if you have multiple nested lists you will need to alter the first list character when repeating them, otherwise you get a weird bit of inconsistent vertical whitespace. To see this, in the current plugin README compare the vertical whitespace difference between the docker measurement and the docker_data. We usually use - and +.

Bad:

- foo
  - bar
- foo2
  - bar2
- foo3
  - bar3
- foo4
  - bar4

Good:

- foo
  - bar
+ foo2
  - bar2
- foo3
  - bar3
+ foo4
  - bar4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah that is weird. Nice I did not know this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What have you done. Now I can't unsee the whitespace 😂 :trollface:

@danielnelson danielnelson self-requested a review July 10, 2019 18:22
@GeorgeMac GeorgeMac merged commit 3f424b8 into influxdata:master Jul 15, 2019
@GeorgeMac GeorgeMac deleted the gm/docker-input-thinpool branch July 15, 2019 09:24
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docker feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get Docker Thin Pool Minimum Free Space Metric Gathered
3 participants