Rework process collector #456
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grobie
approved these changes
Sep 6, 2018
prometheus/process_collector_test.go
Outdated
| close(ch) | ||
| }() | ||
| for m := range ch { | ||
| fmt.Println("feep") |
There was a problem hiding this comment.
Errr… probably just to test the reviewer. :o)
There was a problem hiding this comment.
Will remove before merging.
This unifies both constructors in one with an options argument. The options argument allows to switch on error reporting, as discussed in #219. The change of the contructor signature is breaking, but only mildly so. Plus, the process collector is rarely used explicitly. I used Sourcegraph to search for public usages, with the following results: - 2 occurrences of NewProcessCollectorPIDFn, once in @discordianfish's glimpse, once in @fabxc's etcd_exporter (deprecated anyway). Both are Prom veterans and will simply do the one line change if needed. - 8 occurrences of NewProcessCollector, of which 7 are of the form NewProcessCollector(os.Getpid(), "") Thus, it's a very easy change, which I even hinted at in the doc comment. Signed-off-by: beorn7 <[email protected]>
beorn7
force-pushed
the
beorn7/process_collector
branch
from
1b36154 to
773f502
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This unifies both constructors in one with an options argument.
The options argument allows to switch on error reporting, as discussed
in #219.
The change of the contructor signature is breaking, but only mildly
so. Plus, the process collector is rarely used explicitly. I used
Sourcegraph to search for public usages, with the following results:
glimpse, once in @fabxc's etcd_exporter (deprecated anyway). Both
are Prom veterans and will simply do the one line change if needed.
NewProcessCollector(os.Getpid(), "")
Thus, it's a very easy change, which I even hinted at in the doc
comment.
Signed-off-by: beorn7 [email protected]
Fixes #219.