From 5f27ac3583630ddd2c673b5dd96a4f8fb21bc39f Mon Sep 17 00:00:00 2001 From: Julien Pivotto Date: Mon, 27 Jan 2020 10:53:10 +0100 Subject: [PATCH] Refactor query log fields (#6694) * Refactor query log fields Signed-off-by: Julien Pivotto --- cmd/prometheus/query_log_test.go | 44 ++++++++++++++++++++++---------- promql/engine.go | 20 +++++++-------- promql/engine_test.go | 16 ++++++------ rules/manager.go | 8 +++--- util/httputil/context.go | 10 +++++--- 5 files changed, 59 insertions(+), 39 deletions(-) diff --git a/cmd/prometheus/query_log_test.go b/cmd/prometheus/query_log_test.go index 0797fd78f..81a5040ab 100644 --- a/cmd/prometheus/query_log_test.go +++ b/cmd/prometheus/query_log_test.go @@ -146,24 +146,27 @@ func (p *queryLogTest) queryString() string { // test parameters. func (p *queryLogTest) validateLastQuery(t *testing.T, ql []queryLogLine) { q := ql[len(ql)-1] - testutil.Equals(t, q["query"].(string), p.queryString()) - switch p.origin { - case consoleOrigin: - testutil.Equals(t, q["path"].(string), p.prefix+"/consoles/test.html") - case apiOrigin: - testutil.Equals(t, q["path"].(string), p.prefix+"/api/v1/query") - case ruleOrigin: - testutil.Equals(t, q["groupName"].(string), "querylogtest") - testutil.Equals(t, q["groupFile"].(string), filepath.Join(p.cwd, "testdata", "rules", "test.yml")) - default: - panic("unknown origin") - } + testutil.Equals(t, p.queryString(), q.Params.Query) + testutil.Equals(t, 0, q.Params.Step) + if p.origin != ruleOrigin { host := p.host if host == "[::1]" { host = "::1" } - testutil.Equals(t, q["clientIP"].(string), host) + testutil.Equals(t, host, q.Request.ClientIP) + } + + switch p.origin { + case apiOrigin: + testutil.Equals(t, p.prefix+"/api/v1/query", q.Request.Path) + case consoleOrigin: + testutil.Equals(t, p.prefix+"/consoles/test.html", q.Request.Path) + case ruleOrigin: + testutil.Equals(t, "querylogtest", q.RuleGroup.Name) + testutil.Equals(t, filepath.Join(p.cwd, "testdata", "rules", "test.yml"), q.RuleGroup.File) + default: + panic("unknown origin") } } @@ -342,7 +345,20 @@ func (p *queryLogTest) run(t *testing.T) { } } -type queryLogLine map[string]interface{} +type queryLogLine struct { + Params struct { + Query string `json:"query"` + Step int `json:"step"` + } `json:"params"` + Request struct { + Path string `json:"path"` + ClientIP string `json:"clientIP"` + } `json:"httpRequest"` + RuleGroup struct { + File string `json:"file"` + Name string `json:"name"` + } `json:"ruleGroup"` +} // readQueryLog unmarshal a json-formatted query log into query log lines. func readQueryLog(t *testing.T, path string) []queryLogLine { diff --git a/promql/engine.go b/promql/engine.go index d43fb93f4..0b6468f92 100644 --- a/promql/engine.go +++ b/promql/engine.go @@ -437,20 +437,20 @@ func (ng *Engine) exec(ctx context.Context, q *query) (v Value, w storage.Warnin defer func() { ng.queryLoggerLock.RLock() if l := ng.queryLogger; l != nil { - f := []interface{}{"query", q.q} + params := make(map[string]interface{}, 4) + params["query"] = q.q + if eq, ok := q.Statement().(*EvalStmt); ok { + params["start"] = formatDate(eq.Start) + params["end"] = formatDate(eq.End) + params["step"] = eq.Interval + } + f := []interface{}{"params", params} if err != nil { f = append(f, "error", err) } - if eq, ok := q.Statement().(*EvalStmt); ok { - f = append(f, - "start", formatDate(eq.Start), - "end", formatDate(eq.End), - "step", eq.Interval.String(), - ) - } f = append(f, "stats", stats.NewQueryStats(q.Stats())) if origin := ctx.Value(queryOrigin); origin != nil { - for k, v := range origin.(map[string]string) { + for k, v := range origin.(map[string]interface{}) { f = append(f, k, v) } } @@ -2096,7 +2096,7 @@ func shouldDropMetricName(op ItemType) bool { } // NewOriginContext returns a new context with data about the origin attached. -func NewOriginContext(ctx context.Context, data map[string]string) context.Context { +func NewOriginContext(ctx context.Context, data map[string]interface{}) context.Context { return context.WithValue(ctx, queryOrigin, data) } diff --git a/promql/engine_test.go b/promql/engine_test.go index 7dcf42f44..87426a3aa 100644 --- a/promql/engine_test.go +++ b/promql/engine_test.go @@ -1154,13 +1154,13 @@ func TestQueryLogger_basic(t *testing.T) { f1 := NewFakeQueryLogger() engine.SetQueryLogger(f1) queryExec() - for i, field := range []string{"query", "test statement"} { - testutil.Assert(t, f1.logs[i].(string) == field, "expected %v as key, got %v", field, f1.logs[i]) + for i, field := range []interface{}{"params", map[string]interface{}{"query": "test statement"}} { + testutil.Equals(t, field, f1.logs[i]) } l := len(f1.logs) queryExec() - testutil.Assert(t, 2*l == len(f1.logs), "expected %d fields in logs, got %v", 2*l, len(f1.logs)) + testutil.Equals(t, 2*l, len(f1.logs)) // Test that we close the query logger when unsetting it. testutil.Assert(t, !f1.closed, "expected f1 to be open, got closed") @@ -1194,7 +1194,7 @@ func TestQueryLogger_fields(t *testing.T) { engine.SetQueryLogger(f1) ctx, cancelCtx := context.WithCancel(context.Background()) - ctx = NewOriginContext(ctx, map[string]string{"foo": "bar"}) + ctx = NewOriginContext(ctx, map[string]interface{}{"foo": "bar"}) defer cancelCtx() query := engine.newTestQuery(func(ctx context.Context) error { return contextDone(ctx, "test statement execution") @@ -1206,7 +1206,7 @@ func TestQueryLogger_fields(t *testing.T) { expected := []string{"foo", "bar"} for i, field := range expected { v := f1.logs[len(f1.logs)-len(expected)+i].(string) - testutil.Assert(t, field == v, "expected %v as key, got %v", field, v) + testutil.Equals(t, field, v) } } @@ -1224,7 +1224,7 @@ func TestQueryLogger_error(t *testing.T) { engine.SetQueryLogger(f1) ctx, cancelCtx := context.WithCancel(context.Background()) - ctx = NewOriginContext(ctx, map[string]string{"foo": "bar"}) + ctx = NewOriginContext(ctx, map[string]interface{}{"foo": "bar"}) defer cancelCtx() testErr := errors.New("failure") query := engine.newTestQuery(func(ctx context.Context) error { @@ -1234,7 +1234,7 @@ func TestQueryLogger_error(t *testing.T) { res := query.Exec(ctx) testutil.NotOk(t, res.Err, "query should have failed") - for i, field := range []interface{}{"query", "test statement", "error", testErr} { - testutil.Assert(t, f1.logs[i] == field, "expected %v as key, got %v", field, f1.logs[i]) + for i, field := range []interface{}{"params", map[string]interface{}{"query": "test statement"}, "error", testErr} { + testutil.Equals(t, f1.logs[i], field) } } diff --git a/rules/manager.go b/rules/manager.go index c15b2e652..cbb89df79 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -287,9 +287,11 @@ func (g *Group) run(ctx context.Context) { return } - ctx = promql.NewOriginContext(ctx, map[string]string{ - "groupFile": g.File(), - "groupName": g.Name(), + ctx = promql.NewOriginContext(ctx, map[string]interface{}{ + "ruleGroup": map[string]string{ + "file": g.File(), + "name": g.Name(), + }, }) iter := func() { diff --git a/util/httputil/context.go b/util/httputil/context.go index 617e2a9d3..b32fb9c57 100644 --- a/util/httputil/context.go +++ b/util/httputil/context.go @@ -42,9 +42,11 @@ func ContextFromRequest(ctx context.Context, r *http.Request) (context.Context, if v := ctx.Value(pathParam); v != nil { path = v.(string) } - return promql.NewOriginContext(ctx, map[string]string{ - "clientIP": ip, - "method": r.Method, - "path": path, + return promql.NewOriginContext(ctx, map[string]interface{}{ + "httpRequest": map[string]string{ + "clientIP": ip, + "method": r.Method, + "path": path, + }, }), nil }