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

Add zfs pool stats collection. #427

Closed
wants to merge 3 commits into from

Conversation

allenpetersen
Copy link
Contributor

Add optional support for gathering zfs pool iostats.

The "two pool, one metic" test was only passing because of previous calls to Gather() had already populated the values.
@allenpetersen
Copy link
Contributor Author

I noticed there was an issue with the original zfs test where it really wasn't testing a single dataset. The second commit addresses this.

@@ -32,7 +37,7 @@ func (z *Zfs) Description() string {
return "Read metrics of ZFS from arcstats, zfetchstats and vdev_cache_stats"
}

func getTags(kstatPath string) map[string]string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep getTags and make getPoolStats a separate function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know it isn't ideal, I wanted to avoid doing the directory scan a second time for each Gather call.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see what you mean, one thing you could do is make tags a private variable on the Zfs struct. Then you can set z.tags["pool"] = ... within this function, and don't return a map in that case.

You can either pass in the Zfs struct or make this function have a receiver (z *Zfs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about putting the directory scan into a getPools() that returns a slice of structs containing the pool name and io path. Then the getPoolsTag() and gatherPoolStats() will both take this this as the input. Do you like that better than adding a tags property to Zfs?

Copy link
Contributor

Choose a reason for hiding this comment

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

@allenpetersen that works as well, 👍

@sparrc
Copy link
Contributor

sparrc commented Dec 10, 2015

thanks @allenpetersen! Just a few comments to address and I can get this merged

@allenpetersen
Copy link
Contributor Author

@sparrc I think this cleans things up. I didn't love my first pass but I was trying to minimize the amount of change to the existing code.

@sparrc sparrc closed this in c89ef84 Dec 11, 2015
@sparrc
Copy link
Contributor

sparrc commented Dec 11, 2015

thanks again @allenpetersen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants