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.

Sign up for GitHub

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

Jump to bottom

Add dataset metrics to zfs input #8383

Merged
merged 3 commits into from
Nov 27, 2020
Merged

Add dataset metrics to zfs input #8383

merged 3 commits into from
Nov 27, 2020

Conversation

zozoh94
Copy link
Contributor

@zozoh94 zozoh94 commented Nov 10, 2020 •
edited
Loading

Required for all PRs:

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

@GilgOuille
Copy link

Wow, excellent !

This is exactly what I need ! Very thanks @zozoh94 !

@srebhan srebhan self-assigned this Nov 13, 2020
srebhan
srebhan requested changes Nov 13, 2020
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Nice work. However, I think we can simplify this a bit more, see comments.

srebhan
srebhan reviewed Nov 21, 2020
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Almost there. :-) Could you please comment or fix the one comment on logging dropped lines!

I have one more general question tough: Your PR adds dataset-collection to FreeBSD but it sould also work on e.g. Linux. What about those poor souls? Do you think you can make it also work on Linux? I'll be able to test it here...

@zozoh94
Copy link
Contributor Author

zozoh94 commented Nov 23, 2020

@srebhan I added a log but I'm sorry but I have no experience with ZFS on linux.

@srebhan
Copy link
Member

srebhan commented Nov 23, 2020

Hey @zozoh94, I think linux behaves exactly the same as *BSD there. When calling zfs list -Hp -o name,avail,used,usedsnap,usedds on linux I get

mytestpool      7318564864      482809856       0       482642432
mytestpool/mydataset1   7318564864      24576   0       24576
mytestpool/mydataset2   7318564864      24576   0       24576
mytestpool/mydataset3   7318564864      24576   0       24576

as output. So I guess it's pretty much the same you're seeing on FreeBSD!? However, I realized the Linux code relies on directly reading /proc... So never mind.

@sjwang90 sjwang90 added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Nov 24, 2020
@zozoh94 zozoh94 force-pushed the master branch 2 times, most recently from 603b454 to dc49e7a Compare November 25, 2020 16:15
srebhan
srebhan approved these changes Nov 26, 2020
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Nov 26, 2020
ssoroka
ssoroka reviewed Nov 26, 2020
View reviewed changes
Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Looks pretty good. just some minor comments

plugins/inputs/zfs/zfs_freebsd.go Show resolved Hide resolved
plugins/inputs/zfs/zfs_freebsd.go Show resolved Hide resolved
plugins/inputs/zfs/zfs_freebsd.go Outdated Show resolved Hide resolved
@ssoroka ssoroka merged commit ef91f96 into influxdata:master Nov 27, 2020
@ssoroka
Copy link
Contributor

ssoroka commented Nov 27, 2020

Thanks!

@toelke
Copy link

toelke commented Dec 22, 2020 •
edited
Loading

FWIW, the freebsd-code also works with linux, see a crappy port at toelke@fae76e8

arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@zozoh94
Add dataset metrics to zfs input (influxdata#8383)
7cf7481
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srebhan srebhan srebhan approved these changes

@ssoroka ssoroka ssoroka approved these changes

Assignees

@srebhan srebhan

Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zozoh94 @GilgOuille @srebhan @ssoroka @toelke @sjwang90