Skip to content
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

query-frontend: Extract org id from from request headers #3245

Merged
merged 6 commits into from
Nov 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
- [#3398](https://github.com/thanos-io/thanos/pull/3398) Query Frontend: Add default config for query frontend memcached config.
- [#3277](https://github.com/thanos-io/thanos/pull/3277) Thanos Query: Introduce dynamic lookback interval. This allows queries with large step to make use of downsampled data.
- [#3409](https://github.com/thanos-io/thanos/pull/3409) Compactor: Added support for no-compact-mark.json which excludes the block from compaction.
- [#3245](https://github.com/thanos-io/thanos/pull/3245) Query Frontend: Add `query-frontend.org-id-header` flag to specify HTTP header(s) to populate slow query log (e.g. X-Grafana-User).

### Fixed

Expand Down
19 changes: 18 additions & 1 deletion cmd/thanos/query_frontend.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
type queryFrontendConfig struct {
http httpConfig
queryfrontend.Config
orgIdHeaders []string
}

func registerQueryFrontend(app *extkingpin.App) {
Expand Down Expand Up @@ -115,6 +116,11 @@ func registerQueryFrontend(app *extkingpin.App) {
cmd.Flag("query-frontend.log-queries-longer-than", "Log queries that are slower than the specified duration. "+
"Set to 0 to disable. Set to < 0 to enable on all queries.").Default("0").DurationVar(&cfg.CortexFrontendConfig.LogQueriesLongerThan)

cmd.Flag("query-frontend.org-id-header", "Request header names used to identify the source of slow queries (repeated flag). "+
"The values of the header will be added to the org id field in the slow query log. "+
"If multiple headers match the request, the first matching arg specified will take precedence. "+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this bit? Can you add test for this case to make it a bit more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, yeah this sounded clearer in my head; I'll try to make the behaviour clearer in the help string. The idea is that if --query-frontend.org-id-header is used multiple times, we try to extract a header from the request in order the command line args were specified.

There are test cases to check the precedence behaviour in query_frontend_test.go, e.g. we check that when both --query-frontend.org-id-header=x-server-id and query-frontend.org-id-header=x-grafana-user are specified that org id is set to the value of the x-server-id header. However, happy to split that into a few test functions with different names if it would be clearer what's being tested?

Also, I noticed that this file has changed a bit since writing it and after rebasing it seems bit out of place. Considering moving the new logic into pkg/queryfrontend. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying, now looking at the test it is clear.
I think the test location is fine, but if you prefer can move it.

"If no headers match 'anonymous' will be used.").PlaceHolder("<http-header-name>").StringsVar(&cfg.orgIdHeaders)

cmd.Flag("log.request.decision", "Request Logging for logging the start and end of requests. LogFinishCall is enabled by default. LogFinishCall : Logs the finish call of the requests. LogStartAndFinishCall : Logs the start and finish call of the requests. NoLogCall : Disable request logging.").Default("LogFinishCall").EnumVar(&cfg.RequestLoggingDecision, "NoLogCall", "LogFinishCall", "LogStartAndFinishCall")

cmd.Setup(func(g *run.Group, logger log.Logger, reg *prometheus.Registry, tracer opentracing.Tracer, _ <-chan struct{}, _ bool) error {
Expand Down Expand Up @@ -199,6 +205,7 @@ func runQueryFrontend(

instr := func(f http.HandlerFunc) http.HandlerFunc {
hf := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
orgId := extractOrgId(cfg, r)
name := "query-frontend"
api.SetCORS(w)
ins.NewHandler(
Expand All @@ -213,7 +220,7 @@ func runQueryFrontend(
),
),
// Cortex frontend middlewares require orgID.
).ServeHTTP(w, r.WithContext(user.InjectOrgID(r.Context(), "fake")))
).ServeHTTP(w, r.WithContext(user.InjectOrgID(r.Context(), orgId)))
})
return hf
}
Expand All @@ -235,3 +242,13 @@ func runQueryFrontend(
statusProber.Ready()
return nil
}

func extractOrgId(conf *queryFrontendConfig, r *http.Request) string {
for _, header := range conf.orgIdHeaders {
headerVal := r.Header.Get(header)
if headerVal != "" {
return headerVal
}
}
return "anonymous"
}
46 changes: 46 additions & 0 deletions cmd/thanos/query_frontend_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright (c) The Thanos Authors.
// Licensed under the Apache License 2.0.

package main

import (
"net/http"
"testing"

"github.com/thanos-io/thanos/pkg/testutil"
)

func Test_extractOrgId(t *testing.T) {
var testData = []struct {
configuredHeaders []string
requestHeaders map[string]string
expectOrgId string
}{
{
configuredHeaders: []string{"x-grafana-user", "x-server-id"},
requestHeaders: map[string]string{"x-grafana-user": "imagrafanauser", "x-server-id": "iamserverid"},
expectOrgId: "imagrafanauser",
},
{
configuredHeaders: []string{"x-server-id", "x-grafana-user"},
requestHeaders: map[string]string{"x-grafana-user": "imagrafanauser", "x-server-id": "iamserverid"},
expectOrgId: "iamserverid",
},
{
configuredHeaders: []string{},
requestHeaders: map[string]string{"another-header": "another"},
expectOrgId: "anonymous",
},
}
for _, data := range testData {
config := queryFrontendConfig{
orgIdHeaders: data.configuredHeaders,
}
req, _ := http.NewRequest("", "", nil)
for k, v := range data.requestHeaders {
req.Header.Set(k, v)
}
id := extractOrgId(&config, req)
testutil.Equals(t, data.expectOrgId, id)
}
}
8 changes: 8 additions & 0 deletions docs/components/query-frontend.md
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,14 @@ Flags:
Log queries that are slower than the specified
duration. Set to 0 to disable. Set to < 0 to
enable on all queries.
--query-frontend.org-id-header=<http-header-name> ...
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yeya24 - thanks for the feedback. Regarding docs, I'm considering adding something to explain the new flag in the slow-query-log section of query-frontend.md. And probably some godoc to explain the new function. Is that the kind of thing you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply add this description to the flag and I think it would be clear 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow. I think it's already like that. This description is the one generated from the flag in cmd/thanos/query_frontend.go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I mean you can add something like This ord id will be added to the slow-query-log to the flag description directly and then regenerate the doc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made an update to the flag description. Hope it's clearer, but open to suggestions for a better way to describe it.

Request header names used to identify the
source of slow queries (repeated flag). The
values of the header will be added to the org
id field in the slow query log. If multiple
headers match the request, the first matching
arg specified will take precedence. If no
headers match 'anonymous' will be used.
--log.request.decision=LogFinishCall
Request Logging for logging the start and end
of requests. LogFinishCall is enabled by
Expand Down