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

Include mount options in disk metrics #3027

Merged
merged 1 commit into from
Sep 6, 2017
Merged

Include mount options in disk metrics #3027

merged 1 commit into from
Sep 6, 2017

Conversation

rul
Copy link
Contributor

@rul rul commented Jul 18, 2017

This patch just adds the mount options to the disk metrics. This is useful for alerting, for example, when a partition gets remounted as read only due an error.

Required for all PRs:

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

@danielnelson
Copy link
Contributor

I'm not a big fan of how the options end up in one tag, maybe we should try to extract certain options as tags. What do you think @desa?

@desa
Copy link
Contributor

desa commented Jul 20, 2017

@danielnelson yeah I'm not a huge fan of that either. I'd just make them separate tags.

@rul
Copy link
Contributor Author

rul commented Jul 21, 2017

I wasn't convinced either, but the problem of having the options as separate tags is that not all of them are of the form name=value. Some of them are just a single string, as rw. Do you have any suggestion on what to do in those cases?

@danielnelson
Copy link
Contributor

That does make it trickier to name the tags, and it looks like many options are just a single string. I wonder if instead of trying to support all opts, we should create tags for a smaller set of understood options, which would allow us to customize how it is stored. If we do this we should probably start small, would it be enough in for your use case if you had mode=rw and mode=ro?

@rul
Copy link
Contributor Author

rul commented Jul 21, 2017

Oh, yeah, for my use case that's enough. I'll try to update the PR with these changes.

@rul
Copy link
Contributor Author

rul commented Sep 6, 2017

@danielnelson I've updated this PR with your suggestion

@danielnelson danielnelson added this to the 1.5.0 milestone Sep 6, 2017
@danielnelson danielnelson merged commit 99dfc69 into influxdata:master Sep 6, 2017
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.

3 participants