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

Use uint64 in the ZFS collector #714

Merged
merged 1 commit into from
Jan 6, 2018

Conversation

fpletz
Copy link
Contributor

@fpletz fpletz commented Oct 25, 2017

ZFS metrics can also be unsigned 64-bit integers that won't fit in int64 and causes the whole collector to fail.

Example

time="2017-10-25T17:28:16Z" level=error msg="ERROR: zfs collector failed after 0.000335s: could not parse expected integer value for \"kstat.zfs.misc.zil.zil_itx_needcopy_bytes\"" source="collector.go:123"

# grep zil_itx_needcopy_bytes /proc/spl/kstat/zfs/zil 
zil_itx_needcopy_bytes          4    18446744073709037523

The internal ZFS datatype kstat_named_t can have an uint64 value: https://github.com/zfsonlinux/zfs/blob/master/lib/libspl/include/sys/kstat.h#L472

Why float64 and not uint64?

Prometheus uses float64 internally and the metrics would be converted to float64 eventually anyway.

@joehandzik
Copy link
Contributor

This looks good to me, thanks for the catch. We saw cases like this in our Lustre Exporter, but hadn't in the ZFS code.

@fpletz
Copy link
Contributor Author

fpletz commented Oct 25, 2017

The travis error is unrelated. Travis was apparently rate-limited by go.googlesource.com.

fpletz added a commit to mayflower/nixpkgs that referenced this pull request Oct 25, 2017
@SuperQ
Copy link
Member

SuperQ commented Oct 25, 2017

Rather than changing to float64, we could parse the proc file with uint64. We have done this in several other parts of the node_exporter/procfs library.

@joehandzik
Copy link
Contributor

@SuperQ Any particular reason you'd advise avoiding the use of floats by default? We've erred on the side of just creating floats by default to prevent this sort of problem in other Go code. I haven't done any performance analysis comparing the two, curious if you have particularly strong reasons for what you suggested.

@SuperQ
Copy link
Member

SuperQ commented Oct 25, 2017

Mainly, I am aiming for correctness of parsing the files exposed by the kernel. ParseFloat() on a proc file could, in theory, give us strange results. This zfs parsing code should eventually be moved to procfs, where we would parse with uint64 since it would be a not-Prometheus-specific library.

It's not something that will happen any time soon, but there are vague ideas that we may support uint64 natively in the future at the metrics library level. For better compatibility with other metrics systems that do support it.

@SuperQ
Copy link
Member

SuperQ commented Oct 25, 2017

An example of how we want to go is the xfs parser.

@joehandzik
Copy link
Contributor

@SuperQ Excellent clarification, thank you! @fpletz, sounds like it'd be for the best to just go with uint64 in this PR. I'll take a look at pulling the ZFS metrics into the procfs project.

@fpletz
Copy link
Contributor Author

fpletz commented Oct 25, 2017

Sounds reasonable, thanks for the explanation! I'll change this PR accordingly.

@SuperQ
Copy link
Member

SuperQ commented Oct 25, 2017

If you want some real fun, you could port this code over to the procfs library. 😜

@discordianfish
Copy link
Member

@fpletz As @SuperQ said, happy to merge it with uin64 but even better if you would move it to https://github.com/prometheus/procfs

@discordianfish
Copy link
Member

@fpletz Can you change it to uint64 so we can merge it.
Otherwise I'll close this PR for now.

ZFS metrics can also be unsigned 64-bit integers that won't fit in
int64 and causes the whole collector to fail.
@fpletz fpletz changed the title Use float64 in the ZFS collector Use uint64 in the ZFS collector Jan 4, 2018
@fpletz
Copy link
Contributor Author

fpletz commented Jan 4, 2018

@discordianfish Thanks for the reminder. Rebased and changed to uint64. Tests are green. Ready to merge.

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

@discordianfish
Copy link
Member

Great, thanks! LGTM!

@discordianfish discordianfish merged commit d432f98 into prometheus:master Jan 6, 2018
@fpletz fpletz deleted the fix/zfs-uint64-stats branch March 5, 2018 15:56
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
ZFS metrics can also be unsigned 64-bit integers that won't fit in
int64 and causes the whole collector to fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants