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

Refactor toplevel functions #162

Merged
merged 2 commits into from
May 30, 2019
Merged

Refactor toplevel functions #162

merged 2 commits into from
May 30, 2019

Conversation

pgier
Copy link
Collaborator

@pgier pgier commented May 21, 2019

See issue #44

@pgier pgier force-pushed the refactor-toplevel-functions branch from b61cca1 to 4e56a4f Compare May 21, 2019 17:13
mdlayher
mdlayher approved these changes May 21, 2019
View reviewed changes
Copy link
Contributor

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM with a couple small nits.

mdstat.go Outdated Show resolved Hide resolved
proc_psi.go Outdated Show resolved Hide resolved
@pgier pgier force-pushed the refactor-toplevel-functions branch from 02ccb3e to 1951885 Compare May 21, 2019 20:43
mdlayher
mdlayher reviewed May 21, 2019
View reviewed changes
mdstat.go Outdated Show resolved Hide resolved
mdstat.go Outdated Show resolved Hide resolved
@pgier pgier force-pushed the refactor-toplevel-functions branch from db4f258 to f7fc61d Compare May 21, 2019 20:52
mdlayher
mdlayher reviewed May 21, 2019
View reviewed changes
xfs/xfs.go Outdated Show resolved Hide resolved
@pgier
Copy link
Collaborator Author

pgier commented May 24, 2019

@discordianfish Does this look ok to you?

pgier added 2 commits May 29, 2019 13:30
@pgier
remove top level constructor functions
c66a36f 
This removes top level 'New<Thing>' constructor functions
in order to make the API slightly smaller and more consistent.
Fixes issue prometheus#44

Also includes some minor readme and godoc updates.

Signed-off-by: Paul Gier <pgier@redhat.com>
@pgier
mdstat.go: split file I/O and parsing
b9e9d29 
Split the file I/O and parsing into two separate functions
for better maintenance.  Also some minor refactoring/cleanup.

Signed-off-by: Paul Gier <pgier@redhat.com>
@pgier pgier force-pushed the refactor-toplevel-functions branch from 72438fa to b9e9d29 Compare May 29, 2019 21:16
@pgier
Copy link
Collaborator Author

pgier commented May 29, 2019 •
edited

Rebased and cleaned up the commits.

SuperQ
SuperQ approved these changes May 30, 2019
View reviewed changes
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.

LGTM

@pgier pgier merged commit 0c11a8e into prometheus:master May 30, 2019
@pgier pgier mentioned this pull request May 30, 2019
Closed
@sbramin
Copy link

sbramin commented May 30, 2019 •
edited

Hello @pgier

This breaks the client_golang pkg as NewStat has been renamed to Stat
go/src/github.com/prometheus/client_golang/prometheus/process_collector.go:169:20: p.NewStat undefined (type procfs.Proc has no field or method NewStat)

unless you are making changes to that at the same time?

edit*

Docs also still reference NewStat

@peterbourgon
Copy link

:sad:

This was referenced May 30, 2019
Closed
Closed
@pgier
Copy link
Collaborator Author

pgier commented May 30, 2019 •
edited

@sbramin @peterbourgon The plan is to update client_golang to match this updated API. client_golang should still be pointing to the older commit in its module config, are you automatically updating to latest?
If needed, we could tag a release version before this change, or I can revert if this is causing a problem.
Or I can re-add NewStat and just mark it as deprecated.

@sbramin
Copy link

sbramin commented May 30, 2019

Depends on the expected time frame to update client_golang, If its a matter of hours then I can sit on my hands, if its days then perhaps its better to have procfs/golang_client being released at the same time? As I am sure I am not the only cowboy running off of master. Re-adding a deprecated section sounds like more trouble than its worth.

@peterbourgon
Copy link

Modules are not universally used. My vote would be for

  • Revert
  • Add new constructors to procfs
  • Migrate client_golang to new constructors
  • Remove old constructors

@peterbourgon
Copy link

Something needs to be done immediately, the Prometheus universe is currently broken.

@pgier pgier mentioned this pull request May 30, 2019
Merged
bobrik pushed a commit to bobrik/procfs that referenced this pull request Jan 14, 2023
@eminence
Merge pull request prometheus#162 from pymongo/proc_net_route
7b70f1d 
Add support for /proc/net/route
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdlayher mdlayher mdlayher approved these changes

@SuperQ SuperQ SuperQ approved these changes

@discordianfish discordianfish Awaiting requested review from discordianfish

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pgier @sbramin @peterbourgon @SuperQ @mdlayher