-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Changes from all commits
0889173
f0d3948
e8e592e
35dae03
1b505ae
c49798e
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 |
---|---|---|
@@ -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) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> ... | ||
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. @yeya24 - thanks for the feedback. Regarding docs, I'm considering adding something to explain the new flag in the 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. You can simply add this description to the flag and I think it would be clear 👍 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. 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. 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. Yes, I mean you can add something like 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. 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 | ||
|
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.
I don't understand this bit? Can you add test for this case to make it a bit more clear?
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.
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
andquery-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?
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.
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.