From 6b70a4d85031ecfb1404c3702237ebc0e7d3f2c8 Mon Sep 17 00:00:00 2001 From: Goutham Veeramachaneni Date: Fri, 16 Jun 2017 16:44:33 +0530 Subject: [PATCH] Incorporate PR feedback * Move fingerprint to Hash() * Move away from tsdb.MultiError * 0777 -> 0666 for files * checkOverflow of extra fields Signed-off-by: Goutham Veeramachaneni --- cmd/promtool/main.go | 20 +++++++++++--------- pkg/rulefmt/rulefmt.go | 35 +++++++++++++++++++++++++++++++++++ rules/manager.go | 34 +++++++++++++++++++--------------- 3 files changed, 65 insertions(+), 24 deletions(-) diff --git a/cmd/promtool/main.go b/cmd/promtool/main.go index 73e2698a3..6e5dbf029 100644 --- a/cmd/promtool/main.go +++ b/cmd/promtool/main.go @@ -29,7 +29,6 @@ import ( "github.com/prometheus/prometheus/promql" "github.com/prometheus/prometheus/util/cli" "github.com/prometheus/prometheus/util/promlint" - "github.com/prometheus/tsdb" ) // CheckConfigCmd validates configuration files. @@ -153,8 +152,11 @@ func CheckRulesCmd(t cli.Term, args ...string) int { failed := false for _, arg := range args { - if n, err := checkRules(t, arg); err != nil { - t.Errorf(" FAILED: %s", err) + if n, errs := checkRules(t, arg); errs != nil { + t.Errorf(" FAILED:") + for _, e := range errs { + t.Errorf(e.Error()) + } failed = true } else { t.Infof(" SUCCESS: %d rules found", n) @@ -167,18 +169,18 @@ func CheckRulesCmd(t cli.Term, args ...string) int { return 0 } -func checkRules(t cli.Term, filename string) (int, error) { +func checkRules(t cli.Term, filename string) (int, []error) { t.Infof("Checking %s", filename) if stat, err := os.Stat(filename); err != nil { - return 0, fmt.Errorf("cannot get file info") + return 0, []error{fmt.Errorf("cannot get file info")} } else if stat.IsDir() { - return 0, fmt.Errorf("is a directory") + return 0, []error{fmt.Errorf("is a directory")} } rgs, errs := rulefmt.ParseFile(filename) if errs != nil { - return 0, tsdb.MultiError(errs) + return 0, errs } numRules := 0 @@ -265,7 +267,7 @@ func updateRules(t cli.Term, filename string) error { return err } - return ioutil.WriteFile(filename+".yaml", y, 0777) + return ioutil.WriteFile(filename+".yaml", y, 0666) } var checkMetricsUsage = strings.TrimSpace(` @@ -329,7 +331,7 @@ func main() { }) app.Register("update-rules", &cli.Command{ - Desc: "update the rules to the new YAML format", + Desc: "update rules to the new YAML format", Run: UpdateRulesCmd, }) diff --git a/pkg/rulefmt/rulefmt.go b/pkg/rulefmt/rulefmt.go index 3afb1bbfd..81a5962c0 100644 --- a/pkg/rulefmt/rulefmt.go +++ b/pkg/rulefmt/rulefmt.go @@ -14,7 +14,9 @@ package rulefmt import ( + "fmt" "io/ioutil" + "strings" "github.com/pkg/errors" "github.com/prometheus/common/model" @@ -37,6 +39,9 @@ func (err *Error) Error() string { type RuleGroups struct { Version int `yaml:"version"` Groups []RuleGroup `yaml:"groups"` + + // Catches all undefined fields and must be empty after parsing. + XXX map[string]interface{} `yaml:",inline"` } // Validate validates all rules in the rule groups. @@ -58,6 +63,10 @@ func (g *RuleGroups) Validate() (errs []error) { ) } + if err := checkOverflow(g.XXX, "rule_group"); err != nil { + errs = append(errs, errors.Wrapf(err, "Group: %s", g.Name)) + } + set[g.Name] = struct{}{} for i, r := range g.Rules { @@ -70,6 +79,11 @@ func (g *RuleGroups) Validate() (errs []error) { } } } + + if err := checkOverflow(g.XXX, "config_file"); err != nil { + errs = append(errs, err) + } + return errs } @@ -78,6 +92,9 @@ type RuleGroup struct { Name string `yaml:"name"` Interval model.Duration `yaml:"interval,omitempty"` Rules []Rule `yaml:"rules"` + + // Catches all undefined fields and must be empty after parsing. + XXX map[string]interface{} `yaml:",inline"` } // Rule describes an alerting or recording rule. @@ -88,6 +105,9 @@ type Rule struct { For model.Duration `yaml:"for,omitempty"` Labels map[string]string `yaml:"labels,omitempty"` Annotations map[string]string `yaml:"annotations,omitempty"` + + // Catches all undefined fields and must be empty after parsing. + XXX map[string]interface{} `yaml:",inline"` } // Validate the rule and return a list of encountered errors. @@ -129,9 +149,24 @@ func (r *Rule) Validate() (errs []error) { } } + if err := checkOverflow(r.XXX, "rule"); err != nil { + errs = append(errs, err) + } + return errs } +func checkOverflow(m map[string]interface{}, ctx string) error { + if len(m) > 0 { + var keys []string + for k := range m { + keys = append(keys, k) + } + return fmt.Errorf("unknown fields in %s: %s", ctx, strings.Join(keys, ", ")) + } + return nil +} + // ParseFile parses the rule file and validates it. func ParseFile(file string) (*RuleGroups, []error) { b, err := ioutil.ReadFile(file) diff --git a/rules/manager.go b/rules/manager.go index 42928ef6a..a72cfe777 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -14,6 +14,7 @@ package rules import ( + "errors" "fmt" "math" "net/url" @@ -26,8 +27,6 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/log" - "github.com/prometheus/common/model" - "github.com/prometheus/tsdb" "golang.org/x/net/context" "github.com/prometheus/prometheus/config" @@ -212,12 +211,13 @@ func (g *Group) stop() { <-g.terminated } -func (g *Group) fingerprint() model.Fingerprint { - l := model.LabelSet{ - "name": model.LabelValue(g.name), - "filename": model.LabelValue(g.file), - } - return l.Fingerprint() +func (g *Group) hash() uint64 { + l := labels.New( + labels.Label{"name", g.name}, + labels.Label{"file", g.file}, + ) + + return l.Hash() } // offset returns until the next consistently slotted evaluation interval. @@ -226,7 +226,7 @@ func (g *Group) offset() time.Duration { var ( base = now - (now % int64(g.interval)) - offset = uint64(g.fingerprint()) % uint64(g.interval) + offset = g.hash() % uint64(g.interval) next = base + int64(offset) ) @@ -466,9 +466,13 @@ func (m *Manager) ApplyConfig(conf *config.Config) error { } // To be replaced with a configurable per-group interval. - groups, err := m.loadGroups(time.Duration(conf.GlobalConfig.EvaluationInterval), files...) - if err != nil { - return fmt.Errorf("error loading rules, previous rule set restored: %s", err) + groups, errs := m.loadGroups(time.Duration(conf.GlobalConfig.EvaluationInterval), files...) + if errs != nil { + for _, e := range errs { + // TODO(gouthamve): Move to non-global logger. + log.Errorln(e) + } + return errors.New("error loading rules, previous rule set restored") } var wg sync.WaitGroup @@ -511,13 +515,13 @@ func (m *Manager) ApplyConfig(conf *config.Config) error { // loadGroups reads groups from a list of files. // As there's currently no group syntax a single group named "default" containing // all rules will be returned. -func (m *Manager) loadGroups(interval time.Duration, filenames ...string) (map[string]*Group, error) { +func (m *Manager) loadGroups(interval time.Duration, filenames ...string) (map[string]*Group, []error) { groups := make(map[string]*Group) for _, fn := range filenames { rgs, errs := rulefmt.ParseFile(fn) if errs != nil { - return nil, tsdb.MultiError(errs) + return nil, errs } for _, rg := range rgs.Groups { @@ -530,7 +534,7 @@ func (m *Manager) loadGroups(interval time.Duration, filenames ...string) (map[s for _, r := range rg.Rules { expr, err := promql.ParseExpr(r.Expr) if err != nil { - return nil, err + return nil, []error{err} } if r.Alert != "" {