Soft interface #2
Conversation
Signed-off-by: matt durham <mattdurham@ppog.org>
Signed-off-by: matt durham <mattdurham@ppog.org>
Signed-off-by: matt durham <mattdurham@ppog.org>
Signed-off-by: matt durham <mattdurham@ppog.org>
Signed-off-by: matt durham <mattdurham@ppog.org>
Signed-off-by: matt durham <mattdurham@ppog.org>
Signed-off-by: matt durham <mattdurham@ppog.org>
Signed-off-by: matt durham <mattdurham@ppog.org>
exporter/exporter.go
Outdated
| @@ -43,7 +38,7 @@ type prometheusVersion struct { | |||
| } | |||
|
|
|||
| const ( | |||
| defaultCollectors = "cpu,cs,logical_disk,net,os,service,system,textfile" | |||
There was a problem hiding this comment.
Did you mean to remove textfile here?
exporter/exporter.go
Outdated
| t := time.Now() | ||
| cs := make([]string, 0, len(coll.collectors)) | ||
| for name := range coll.collectors { | ||
| collectors := make([]collector.Collector, 0, len(coll.Collectors)) |
There was a problem hiding this comment.
It doesn't look like this variable is being used anywhere, am i missing something?
| var exchangeList = Config{ | ||
| Name: "collectors.exchange.list", | ||
| HelpText: "List the collectors along with their perflib object name/ids", | ||
| Default: "false", | ||
| } | ||
| var exchangeEnabled = Config{ | ||
| Name: "collectors.exchange.enabled", | ||
| HelpText: "Comma-separated list of collectors to use. Defaults to all, if not specified.", | ||
| Default: "", | ||
| } |
There was a problem hiding this comment.
Exposing the configs this way makes me nervous:
- It isn't strongly typed, so importers don't know if they got a flag name wrong until runtime
- All usage of kingpin has to be swapped out with this internal config lib, which makes it harder to justify upstream (it has the appearance of a more disruptive change rather than just shifting logic around)
I'd strongly recommend avoiding doing things this way and to use Prometheus' service discovery mechanism as inspiration:
- Create public per-collector config structs with public fields for each tunable
- Add a
RegisterFlagsmethod (or similar) that binds the values of the field to a flag. - Add a
BuildorNewCollectormethod that creates the collector, passing the config to it as an argument.
There was a problem hiding this comment.
Each config needs to pass 3 things: name, default and helptext. Will try to see how to make that work with using the mechanism linked above. Also isnt one of the purposes to have no usage of kingpin in the individual collectors to prevent possible kingpin overlap?
There was a problem hiding this comment.
Each config needs to pass 3 things: name, default and helptext.
IMO only the default is relevant here, but isn't strictly necessary for using as a library.
Also isnt one of the purposes to have no usage of kingpin in the individual collectors to prevent possible kingpin overlap?
Yes, but the method to register flags should accept a kingpin application as an argument to avoid this problem. (You can also just avoid calling the register flags method entirely from the library, like we would do, since we'll be unmarshaling config from YAML instead)
No description provided.