Add collection throttling support #5
Conversation
There was a problem hiding this comment.
Hi,
thanks for your PR! makes sense, even if I would solve the problem with a less frequent scrape interval. If you apply the throttling globally and not per query, this doesn't make sense and I prefer adjusting the scrape interval over the possibility of configuring a throttle interval and accepting this PR. However, if you implement the throttling per metric, it might make sense.
I also added some remarks . Also, please document the config option in the README.md
| gaugevecs: | ||
| # a list of gaugevec definitions | ||
| [ - <gaugevec definition> ... ] |
| } | ||
|
|
||
| // NewOsqueryCollector creates an OsQueryCollector from a given osquery-runner and a set of metric definitions | ||
| func NewOsqueryCollector(r *osquery.OsqueryRunner, m model.Metrics) *OsqueryCollector { | ||
| func NewOsqueryCollector(r *osquery.OsqueryRunner, m model.Metrics, t string) *OsqueryCollector { |
There was a problem hiding this comment.
I'd prefer handling the time.Duration parsing outside the constructor and to pass in a time.Duration. It's not the concern of the constructor to validate the config
| "github.com/zwopir/osquery_exporter/model" | ||
| "github.com/zwopir/osquery_exporter/osquery" | ||
| "osquery_exporter/model" | ||
| "osquery_exporter/osquery" |
There was a problem hiding this comment.
please keep the github.com/zwopir prefix
There was a problem hiding this comment.
Did that to allow build from custom repo/branch, is there any way to keep prefixes while allowing it without having to remove/re-add them? Spent quite some time trying to figure while builds were failing :D
| if err != nil { | ||
| log.Fatalf("could not parse throttle_interval: %s", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
see above, should be done outside the constructor.
| }(col) | ||
| c.throttle.Lock.Lock() | ||
| defer c.throttle.Lock.Unlock() | ||
| if time.Now().After(c.throttle.LastRun.Add(c.throttle.Interval)) { |
There was a problem hiding this comment.
this throttles all metrics, even if just one metric is slow. These should be a throttle state per osquery (based on the same interval for convenience and simplicity of the config).
There was a problem hiding this comment.
Agree, was my initial thought but went with easier approach to speed up a bit. Will take another look.
| "github.com/zwopir/osquery_exporter/collector" | ||
| "github.com/zwopir/osquery_exporter/model" | ||
| "github.com/zwopir/osquery_exporter/osquery" | ||
| "osquery_exporter/collector" |
| @@ -6,7 +6,7 @@ import ( | |||
| "fmt" | |||
| "github.com/prometheus/common/log" | |||
| "os/exec" | |||
| "github.com/zwopir/osquery_exporter/model" | |||
Allow metric collection throttling to reduce load in case of expensive queries.
Example is queries on
systemd_units(osquery-4.7.0) table are pretty expensive.