Add collect[] parameter#699
Conversation
| staticcheck: $(STATICCHECK) | ||
| @echo ">> running staticcheck" | ||
| @$(STATICCHECK) $(pkgs) | ||
| @$(STATICCHECK) -ignore "$(STATICCHECK_IGNORE)" $(pkgs) |
There was a problem hiding this comment.
Please don't introduce staticcheck violations.
There was a problem hiding this comment.
This was discussed on IRC with @beorn7 and @SuperQ
Without this we will get:
node_exporter.go:86:32: prometheus.InstrumentHandlerFunc is deprecated: InstrumentHandlerFunc is deprecated for the same reasons as InstrumentHandler is. Use the tooling provided in package promhttp instead. (SA1019)
Another solution is to use prometheus@v0.8.0 instead of v0.9.0-pre1
There was a problem hiding this comment.
Ok. Please add a comment mentioning InstrumentHandlerFunc and when that ignoring can be removed again.
There was a problem hiding this comment.
Apparently this will also warn in client_golang v0.8.0 as well. :(
collector/collector.go
Outdated
|
|
||
| // NewNodeCollector creates a new NodeCollector | ||
| func NewNodeCollector() (*nodeCollector, error) { | ||
| func NewNodeCollector(filters map[string]bool) (*nodeCollector, error) { |
There was a problem hiding this comment.
This is not a clean interface, leaking implementation internals (for performance reasons or to safe some lines of code I suppose?) into the function signature.
I propose to use NewNodeCollector(filters ...string) (*nodeCollector, error).
collector/collector.go
Outdated
| return nil, err | ||
| } | ||
| collectors[key] = collector | ||
| if len(filters) == 0 || filters[key] { |
There was a problem hiding this comment.
We should error out if someone tries to use a filter which is not loaded. I suggest to not add this code to the collectorState loop but to do another one on top in case any filters are given.
node_exporter.go
Outdated
|
|
||
| nc, err := collector.NewNodeCollector(filters) | ||
| if err != nil { | ||
| log.Fatalf("Couldn't create collector: %s", err) |
There was a problem hiding this comment.
We should certainly not exit here if an invalid filter request is given. Return a normal HTTP response error.
node_exporter.go
Outdated
| registry, | ||
| } | ||
| // Delegate http serving to Prometheus client library, which will call collector.Collect. | ||
| h := promhttp.HandlerFor(gatherers, promhttp.HandlerOpts{}) |
There was a problem hiding this comment.
Why did you remove the handleropts? These are important.
node_exporter.go
Outdated
| log.Infoln("Build context", version.BuildContext()) | ||
|
|
||
| nc, err := collector.NewNodeCollector() | ||
| // This instance is only used to check collector creation and logging |
grobie
left a comment
grobie
left a comment
There was a problem hiding this comment.
Thanks! One more comment, but looks good otherwise.
node_exporter.go
Outdated
| } | ||
|
|
||
| registry := prometheus.NewRegistry() | ||
| registry.MustRegister(nc) |
There was a problem hiding this comment.
This can panic. Let's use the Register function instead and return a server error response if it fails.
There was a problem hiding this comment.
Thanks for pointing this out!
@SuperQ We should fix this in mysqld_exporter as well.
grobie
left a comment
grobie
left a comment
There was a problem hiding this comment.
Thanks a log. Please add some documentation about the new parameters to the README, then we can merge this.
* Add `collect[]` parameter * Add TODo comment about staticcheck ignored * Restore promhttp.HandlerOpts * Log a warning and return HTTP error instead of failing * Check collector existence and status, cleanups * Fix warnings and error messages * Don't panic, return error if collector registration failed * Update README
3 participants
This PR adds a
collect[]URL parameter to filter currently enabled collectors.