Skip to content

Comments

Parsing Mountinfo to get information not in mountstats#173

Merged
pgier merged 6 commits intoprometheus:masterfrom
dipack95:mountinfo
Jun 19, 2019
Merged

Parsing Mountinfo to get information not in mountstats#173
pgier merged 6 commits intoprometheus:masterfrom
dipack95:mountinfo

Conversation

@dipack95
Copy link
Contributor

@dipack95 dipack95 commented Jun 5, 2019

As @SuperQ said here, I've added parsing for mountinfo to procfs, along with test cases for it.

Dipack P Panjabi added 2 commits June 6, 2019 14:53
Signed-off-by: Dipack P Panjabi <dpanjabi@hudson-trading.com>
Signed-off-by: Dipack P Panjabi <dpanjabi@hudson-trading.com>
@dipack95
Copy link
Contributor Author

dipack95 commented Jun 7, 2019

@pgier Can you take a look at this?

@mknapphrt
Copy link
Contributor

This looks good to me but it's up to @pgier or @SuperQ to give a more authoritative review

Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I added some comments in-line.

Signed-off-by: Dipack P Panjabi <dpanjabi@hudson-trading.com>
@dipack95
Copy link
Contributor Author

dipack95 commented Jun 7, 2019

@pgier Thanks for the feedback! I've modified the code to bring them in line with your suggestions.

Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Some minor nits.

mountinfo.go Outdated
@@ -0,0 +1,171 @@
// Copyright 2018 The Prometheus Authors
Copy link
Member

Choose a reason for hiding this comment

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

New files should get the current year. Please update this to 2019.

mountinfo.go Outdated
"strings"
)

var validOptionalFields = map[string]bool{"shared": true, "master": true, "propagate_from": true, "unbindable": true}
Copy link
Member

Choose a reason for hiding this comment

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

Please break up this long line.

var (
  validOptionalFields = map[string]bool{
    "shared": true,
...

Signed-off-by: Dipack P Panjabi <dpanjabi@hudson-trading.com>
@dipack95
Copy link
Contributor Author

@SuperQ I've made the changes as you requested! Thanks for the feedback.

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.

Please apply comment rules to all comments added by this PR.

@@ -0,0 +1,135 @@
// Copyright 2018 The Prometheus Authors
Copy link
Member

Choose a reason for hiding this comment

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

Missed another date change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, my bad. Just fixed it.

Signed-off-by: Dipack P Panjabi <dpanjabi@hudson-trading.com>
@dipack95
Copy link
Contributor Author

@SuperQ Do you think the PR is in an acceptable state right now?

@dipack95
Copy link
Contributor Author

@SuperQ Gentle reminder! :)

proc.go Outdated
@@ -1,4 +1,4 @@
// Copyright 2018 The Prometheus Authors
// Copyright 2019 The Prometheus Authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this copyright date should remain 2018 since the file already existed before this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll change it now

Signed-off-by: Dipack P Panjabi <dpanjabi@hudson-trading.com>
@pgier pgier merged commit 8fcc02d into prometheus:master Jun 19, 2019
@dipack95 dipack95 deleted the mountinfo branch June 19, 2019 18:18
@dipack95
Copy link
Contributor Author

@pgier I am currently using the changes in this PR in my own fork of node_exporter (by replacing the procfs module in the vendor/ folder), and would like to create a PR for that too.

I was hoping that could create a new release of procfs with these changes included, so that there wouldn't be an issue with the modules in node_exporter. Are you planning to do so?

@pgier
Copy link
Collaborator

pgier commented Jun 19, 2019

@dipack95 I'd like to get a couple more PRs merged and then I'll tag a new release probably some time next week.

remijouannet pushed a commit to remijouannet/procfs that referenced this pull request Oct 20, 2022
Parsing Mountinfo to get information not in mountstats
bobrik pushed a commit to bobrik/procfs that referenced this pull request Jan 14, 2023
Build with cargo update -Z minimal-versions
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.

4 participants