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

Update procfs vendored module #996

Closed
hakoerber opened this issue Jul 11, 2018 · 3 comments · Fixed by #998
Closed

Update procfs vendored module #996

hakoerber opened this issue Jul 11, 2018 · 3 comments · Fixed by #998

Comments

@hakoerber
Copy link
Contributor

Hi!

In prometheus/procfs#100, I added support for UDP NFS mounts in the mountstats module. This introduces a new tag called protocol to the NFSTransportStats struct.

I already wrote some code to get this new tag into the node exporter. This made it necessary to update the logic for detecting the same export on multiple distinct mountpoints (see

i := sort.SearchStrings(deviceList, m.Device)
), because the same export can be mounted with different protocols.

It works like this: Currently, the logic checks for the same Device, which is the NFS export name. The change would instead checks for the tuple (Device, Protocol), with both fields being the same.

I saw #993, which also references the duplicate detection logic. Because I already touched that piece of code, I could use the tuple (Device, Protocol, Version) instead.

How do we proceed here? How do we update vendored dependencies?

I would then create a pull request with my changes so we can review them together.

@SuperQ
Copy link
Member

SuperQ commented Jul 11, 2018

Making a PR that updates the vendoring and updates the feature at the same time is the usual method.

Using the suggested tuple sounds fine to me.

@0xArsen
Copy link

0xArsen commented Jul 11, 2018

@hakoerber Do you plan on adding a new tag to prometheus/procfs that supports the version of the nfs mount?

@SuperQ
Copy link
Member

SuperQ commented Jul 11, 2018

@arsenakishev Yes, I think that's part of the plan. We could perhaps merge #994 now, and then we can add the proto change afterwards.

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 a pull request may close this issue.

3 participants