ProcessCollector: rework #219
Comments
|
@beorn7 These will be breaking changes. I believe the ProcessCollector is rarely used manually, but if you want to be super strict about it, we'd need to wait until 0.10, right? |
|
No, “mild” breaking changes are OK for 0.9, i.e. those that affect only rare usage patterns. 0.10 is for those breaking changes that will affect almost everybody. |
beorn7
pushed a commit
that referenced
this issue
Sep 6, 2018
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
pushed a commit
that referenced
this issue
Sep 7, 2018
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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In particular, see https://github.com/prometheus/client_golang/blob/master/prometheus/process_collector.go#L106
However, even if we can handle errors gracefully now, it will not be the case with the DefaultRegistry. Since a process collector is often added as a best-effort collector to a collections that is otherwise critical to be complete, the process collector should only report errors when requested so. To not have a proliferation of constructors, have just one with ProcessCollectorOpts (pudFn, reportErrors, namespace, with default behavior to not report errors, empty string as namespace, and os.GetPid as pidFn).
Also, the collector should use const metrics.
The text was updated successfully, but these errors were encountered: