Make sure we only return one metric per mounted fs by discordianfish · Pull Request #399 · prometheus/node_exporter

discordianfish · 2017-01-03T14:58:26Z

Closes #377

--
Now we should also have tests for that. I've tried enabling the filesystem collector in the end to end tests but failed. Somehow it always showed a difference for my root filesystem. Anyone know why the collector isn't included in the tests?
Either way, we should have end-to-end tests for that, but I don't think it should block this bugfix here either.

discordianfish · 2017-01-03T14:59:05Z

collector/filesystem_common.go

Not sure if that's the way to go. Could also just iterate over stats to check if something was printed already. Thoughts?

First of all, %q is the printf verb for strings, what you want here is %v or %+v. Go vet should complain about this actually.

Creating a string key for every stats will require additional allocations. Ideally we would just use the struct itself as key. Looking at filesystemStats the only field preventing this is labelValues as structs can only be used as keys if their fields are basic types. Looking at labelValues it appears that using a string slice is a weak data type anyway, as there is strong assumption about the order, length and value. I'd suggest to make the assumption explicit and replace labelValues with three explicit string fields "device", "mountpoint", "fstype".

This will ensure that OS specific implementations can't pass the values in a wrong order and allow you to use the struct as key, avoiding any further allocations by this addition.

as structs can only be used as keys if their fields are basic types

Ah! Good to know, using the struct as key was what I tried first. Will convert it.

Here is the the more accurate description of what types can be compared and used as map keys: https://golang.org/ref/spec#Comparison_operators

grobie · 2017-01-03T16:40:52Z

collector/filesystem_common.go

Maybe seen is clearer?

discordianfish · 2017-01-03T17:28:14Z

@grobie Changed as discussed

grobie · 2017-01-03T21:34:40Z

collector/filesystem_common.go

While I believe the change to filesystemsStats was worthwhile anyway, it's actually still possible that we export multiple metrics for the same label set. Unless it's impossible in all os implementations to create structs with different metric values but same label values, we should maybe afterall create a key only based on the label values.

Oh good catch.. Right, value could be different.. While that should be the case, I came up with a better way. What do you think about this?

grobie

I'm fine with the approach. Just a few nits.

grobie · 2017-01-04T14:37:47Z

collector/filesystem_freebsd.go

This needs to be updated.

Yes, I just realize that we don't run circleci for the pull requests which would have catched this (and me breaking master with my meminfo changes). Opened #401 for that.

grobie · 2017-01-04T14:40:36Z

collector/filesystem_linux.go

Why do we need to build filesystemLabels again?

Missed that when changing. Fixed, also renamed 'mpd' to labels to make it more readable.

grobie · 2017-01-04T14:44:45Z

collector/filesystem_common.go

Did you run go fmt? I believe that will enforce a newline between type definitions.

Nope, but added newline manually.

(Nope it doesn't force, I ran go fmt)

grobie

Thanks! Looks good, feel free to merge.

grobie · 2017-01-04T15:26:21Z

collector/filesystem_freebsd.go

Did you add this newline on purpose?

Nope, fixed and going to merge now.

discordianfish commented Jan 3, 2017

View reviewed changes

grobie reviewed Jan 3, 2017

View reviewed changes

collector/filesystem_common.go Outdated

Copy link

Member

grobie Jan 3, 2017

Choose a reason for hiding this comment

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

Maybe seen is clearer?

discordianfish force-pushed the fish-fs-uniq-metric branch from 6495b30 to 9194943 Compare January 3, 2017 17:27

grobie reviewed Jan 3, 2017

View reviewed changes

discordianfish force-pushed the fish-fs-uniq-metric branch from 9194943 to 6a3d1e6 Compare January 4, 2017 11:04

grobie reviewed Jan 4, 2017

View reviewed changes

discordianfish force-pushed the fish-fs-uniq-metric branch from 6a3d1e6 to 285a701 Compare January 4, 2017 15:15

grobie approved these changes Jan 4, 2017

View reviewed changes

Make sure we only return one metric per mounted fs

4c9131b

discordianfish force-pushed the fish-fs-uniq-metric branch from 285a701 to 4c9131b Compare January 4, 2017 15:45

discordianfish merged commit f9d3f83 into prometheus:master Jan 4, 2017

discordianfish deleted the fish-fs-uniq-metric branch January 4, 2017 15:48

pgier mentioned this pull request Feb 21, 2019

Issue collecting duplicate entries from /proc/mounts #377

Closed

Comments

Conversation

discordianfish commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

discordianfish commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grobie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grobie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants