From 595209303aa2130a76db33836a31d20e63b8f83c Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Tue, 14 Nov 2023 18:02:04 -0800 Subject: [PATCH 1/3] fix param_query not logged in query stats log Signed-off-by: Ben Ye --- pkg/frontend/transport/handler.go | 8 ++--- pkg/frontend/transport/handler_test.go | 42 +++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/pkg/frontend/transport/handler.go b/pkg/frontend/transport/handler.go index 080712056e..36263c2c5d 100644 --- a/pkg/frontend/transport/handler.go +++ b/pkg/frontend/transport/handler.go @@ -98,7 +98,7 @@ type Handler struct { } // NewHandler creates a new frontend handler. -func NewHandler(cfg HandlerConfig, roundTripper http.RoundTripper, log log.Logger, reg prometheus.Registerer) http.Handler { +func NewHandler(cfg HandlerConfig, roundTripper http.RoundTripper, log log.Logger, reg prometheus.Registerer) *Handler { h := &Handler{ cfg: cfg, log: log, @@ -407,17 +407,17 @@ func (f *Handler) parseRequestQueryString(r *http.Request, bodyBuf bytes.Buffer) } func formatQueryString(queryString url.Values) (fields []interface{}) { - var queryFields []string + var queryFields []interface{} for k, v := range queryString { // If `query` or `match[]` field exists, we always put it as the last field. if k == "query" || k == "match[]" { - queryFields = []string{fmt.Sprintf("param_%s", k), strings.Join(v, ",")} + queryFields = []interface{}{fmt.Sprintf("param_%s", k), strings.Join(v, ",")} continue } fields = append(fields, fmt.Sprintf("param_%s", k), strings.Join(v, ",")) } if len(queryFields) > 0 { - fields = append(fields, queryFields) + fields = append(fields, queryFields...) } return fields } diff --git a/pkg/frontend/transport/handler_test.go b/pkg/frontend/transport/handler_test.go index d075f5ec4c..6181981287 100644 --- a/pkg/frontend/transport/handler_test.go +++ b/pkg/frontend/transport/handler_test.go @@ -1,12 +1,16 @@ package transport import ( + "bytes" "context" + querier_stats "github.com/cortexproject/cortex/pkg/querier/stats" "io" "net/http" "net/http/httptest" + "net/url" "strings" "testing" + "time" "github.com/go-kit/log" "github.com/pkg/errors" @@ -274,8 +278,44 @@ func TestHandler_ServeHTTP(t *testing.T) { assert.Equal(t, tt.expectedMetrics, count) if tt.additionalMetricsCheckFunc != nil { - tt.additionalMetricsCheckFunc(handler.(*Handler)) + tt.additionalMetricsCheckFunc(handler) } }) } } + +func TestReportQueryStatsFormat(t *testing.T) { + outputBuf := bytes.NewBuffer(nil) + logger := log.NewSyncLogger(log.NewLogfmtLogger(outputBuf)) + handler := NewHandler(HandlerConfig{QueryStatsEnabled: true}, http.DefaultTransport, logger, nil) + + userID := "fake" + queryString := url.Values(map[string][]string{"query": {"up"}}) + req, err := http.NewRequest(http.MethodGet, "http://localhost:8080/prometheus/api/v1/query", nil) + require.NoError(t, err) + resp := &http.Response{ + Header: http.Header{ + "User-Agent": []string{"Grafana"}, + }, + ContentLength: 1000, + } + stats := &querier_stats.QueryStats{ + Stats: querier_stats.Stats{ + WallTime: 3 * time.Second, + FetchedSeriesCount: 100, + FetchedChunksCount: 200, + FetchedSamplesCount: 300, + FetchedChunkBytes: 1024, + FetchedDataBytes: 2048, + }, + } + responseErr := errors.New("foo_err") + handler.reportQueryStats(req, userID, queryString, time.Second, stats, responseErr, http.StatusOK, resp) + + data, err := io.ReadAll(outputBuf) + require.NoError(t, err) + + expectedLog := `level=error msg="query stats" component=query-frontend method=GET path=/prometheus/api/v1/query response_time=1s query_wall_time_seconds=3 fetched_series_count=100 fetched_chunks_count=200 fetched_samples_count=300 fetched_chunks_bytes=1024 fetched_data_bytes=2048 status_code=200 response_size=1000 query_length=2 error=foo_err param_query=up +` + require.Equal(t, expectedLog, string(data)) +} From 6f8f9b9b0afc796ececc054a955c5df42b6d208e Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Tue, 14 Nov 2023 18:09:29 -0800 Subject: [PATCH 2/3] fix lint Signed-off-by: Ben Ye --- CHANGELOG.md | 1 + pkg/frontend/transport/handler_test.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5f2ac7cef..dba1248b2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * [CHANGE] Azure Storage: Upgraded objstore dependency and support Azure Workload Identity Authentication. Added `connection_string` to support authenticating via SAS token. Marked `msi_resource` config as deprecating. #5645 * [FEATURE] Ingester: Add per-tenant new metric `cortex_ingester_tsdb_data_replay_duration_seconds`. #5477 * [ENHANCEMENT] Store Gateway: Added `-store-gateway.enabled-tenants` and `-store-gateway.disabled-tenants` to explicitly enable or disable store-gateway for specific tenants. #5638 +* [BUGFIX] Query Frontend: Fix query string being omitted in query stats log. #5655 ## 1.16.0 in progress diff --git a/pkg/frontend/transport/handler_test.go b/pkg/frontend/transport/handler_test.go index 6181981287..0aabf0e110 100644 --- a/pkg/frontend/transport/handler_test.go +++ b/pkg/frontend/transport/handler_test.go @@ -3,7 +3,6 @@ package transport import ( "bytes" "context" - querier_stats "github.com/cortexproject/cortex/pkg/querier/stats" "io" "net/http" "net/http/httptest" @@ -20,6 +19,8 @@ import ( "github.com/stretchr/testify/require" "github.com/weaveworks/common/httpgrpc" "github.com/weaveworks/common/user" + + querier_stats "github.com/cortexproject/cortex/pkg/querier/stats" ) type roundTripperFunc func(*http.Request) (*http.Response, error) From 08c4a64a6346509c2e0e8fc0812b86b742e6d296 Mon Sep 17 00:00:00 2001 From: Ben Ye Date: Tue, 14 Nov 2023 19:07:11 -0800 Subject: [PATCH 3/3] fix unit test of user agent Signed-off-by: Ben Ye --- pkg/frontend/transport/handler_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/frontend/transport/handler_test.go b/pkg/frontend/transport/handler_test.go index 0aabf0e110..7cf4a4bd34 100644 --- a/pkg/frontend/transport/handler_test.go +++ b/pkg/frontend/transport/handler_test.go @@ -294,10 +294,10 @@ func TestReportQueryStatsFormat(t *testing.T) { queryString := url.Values(map[string][]string{"query": {"up"}}) req, err := http.NewRequest(http.MethodGet, "http://localhost:8080/prometheus/api/v1/query", nil) require.NoError(t, err) + req.Header = http.Header{ + "User-Agent": []string{"Grafana"}, + } resp := &http.Response{ - Header: http.Header{ - "User-Agent": []string{"Grafana"}, - }, ContentLength: 1000, } stats := &querier_stats.QueryStats{ @@ -316,7 +316,7 @@ func TestReportQueryStatsFormat(t *testing.T) { data, err := io.ReadAll(outputBuf) require.NoError(t, err) - expectedLog := `level=error msg="query stats" component=query-frontend method=GET path=/prometheus/api/v1/query response_time=1s query_wall_time_seconds=3 fetched_series_count=100 fetched_chunks_count=200 fetched_samples_count=300 fetched_chunks_bytes=1024 fetched_data_bytes=2048 status_code=200 response_size=1000 query_length=2 error=foo_err param_query=up + expectedLog := `level=error msg="query stats" component=query-frontend method=GET path=/prometheus/api/v1/query response_time=1s query_wall_time_seconds=3 fetched_series_count=100 fetched_chunks_count=200 fetched_samples_count=300 fetched_chunks_bytes=1024 fetched_data_bytes=2048 status_code=200 response_size=1000 query_length=2 user_agent=Grafana error=foo_err param_query=up ` require.Equal(t, expectedLog, string(data)) }