-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cmd/bosun: expr.execute refactor #1775
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,31 +28,36 @@ type State struct { | |
now time.Time | ||
cache *cache.Cache | ||
enableComputations bool | ||
unjoinedOk bool | ||
|
||
autods int | ||
|
||
*Contexts | ||
// OpenTSDB | ||
Search *search.Search | ||
autods int | ||
tsdbContext opentsdb.Context | ||
tsdbQueries []opentsdb.Request | ||
unjoinedOk bool | ||
squelched func(tags opentsdb.TagSet) bool | ||
|
||
// Bosun Internal | ||
|
||
squelched func(tags opentsdb.TagSet) bool | ||
Search *search.Search | ||
History AlertStatusProvider | ||
|
||
// Graphite | ||
graphiteQueries []graphite.Request | ||
graphiteContext graphite.Context | ||
|
||
// LogstashElastic (for pre ES v2) | ||
logstashQueries []elasticOld.SearchSource | ||
logstashHosts LogstashElasticHosts | ||
|
||
// Elastic (for post ES v2) | ||
elasticQueries []elastic.SearchSource | ||
elasticHosts ElasticHosts | ||
|
||
// InfluxDB | ||
InfluxConfig client.Config | ||
} | ||
|
||
History AlertStatusProvider | ||
type Contexts struct { | ||
TSDBContext opentsdb.Context | ||
GraphiteContext graphite.Context | ||
LogstashHosts LogstashElasticHosts | ||
ElasticHosts ElasticHosts | ||
InfluxConfig client.Config | ||
} | ||
|
||
// Alert Status Provider is used to provide information about alert results. | ||
|
@@ -85,26 +90,20 @@ func New(expr string, funcs ...map[string]parse.Func) (*Expr, error) { | |
|
||
// Execute applies a parse expression to the specified OpenTSDB context, and | ||
// returns one result per group. T may be nil to ignore timings. | ||
func (e *Expr) Execute(c opentsdb.Context, g graphite.Context, l LogstashElasticHosts, eh ElasticHosts, influxConfig client.Config, cache *cache.Cache, T miniprofiler.Timer, now time.Time, autods int, unjoinedOk bool, search *search.Search, squelched func(tags opentsdb.TagSet) bool, history AlertStatusProvider) (r *Results, queries []opentsdb.Request, err error) { | ||
func (e *Expr) Execute(contexts *Contexts, cache *cache.Cache, T miniprofiler.Timer, now time.Time, autods int, unjoinedOk bool, search *search.Search, squelched func(tags opentsdb.TagSet) bool, history AlertStatusProvider) (r *Results, queries []opentsdb.Request, err error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we still have 9 args? wow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I can do better on a revision. Will look some more as long as you approve of the direction. That func signature has been making me mad for a while :P |
||
if squelched == nil { | ||
squelched = func(tags opentsdb.TagSet) bool { | ||
return false | ||
} | ||
} | ||
s := &State{ | ||
Expr: e, | ||
cache: cache, | ||
tsdbContext: c, | ||
graphiteContext: g, | ||
logstashHosts: l, | ||
elasticHosts: eh, | ||
InfluxConfig: influxConfig, | ||
now: now, | ||
autods: autods, | ||
unjoinedOk: unjoinedOk, | ||
Search: search, | ||
squelched: squelched, | ||
History: history, | ||
Expr: e, | ||
now: now, | ||
autods: autods, | ||
unjoinedOk: unjoinedOk, | ||
Contexts: contexts, | ||
Search: search, | ||
squelched: squelched, | ||
} | ||
return e.ExecuteState(s, T) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contexts
may be a bit vague. How aboutBackends
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K