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

Fix param_query omitted in query frontend query stats log #5655

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

yeya24
Copy link
Contributor

@yeya24 yeya24 commented Nov 15, 2023

What this PR does:

We found that param_query unabled to be logged in query frontend in latest Cortex version caused by #5622.

The bug is that, queryFields is of type []string, fields is of type []interface{}.

fields = append(fields, queryFields)

This append will add queryFields []string as a single element to fields slice, causing the params unable to be logged.
The correct way is to add key, value as two separate fields using fields = append(fields, queryFields...).

func formatQueryString(queryString url.Values) (fields []interface{}) {
	var queryFields []string
	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, ",")}
			continue
		}
		fields = append(fields, fmt.Sprintf("param_%s", k), strings.Join(v, ","))
	}
	if len(queryFields) > 0 {
		fields = append(fields, queryFields)
	}
	return fields
}

A unit test case has been added to ensure the correctness.

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@yeya24 yeya24 changed the title Fix param_query not logged in query stats log Fix param_query omitted in query frontend query stats log Nov 15, 2023
@yeya24
Copy link
Contributor Author

yeya24 commented Nov 15, 2023

This change will also be cherry-picked into the release branch

Signed-off-by: Ben Ye <benye@amazon.com>
Signed-off-by: Ben Ye <benye@amazon.com>
Signed-off-by: Ben Ye <benye@amazon.com>
@yeya24 yeya24 force-pushed the fix-query-stats-log-qfe branch from 1a3c0c3 to 08c4a64 Compare November 15, 2023 03:08
@yeya24 yeya24 enabled auto-merge (squash) November 15, 2023 03:42
@yeya24 yeya24 merged commit b601141 into cortexproject:master Nov 15, 2023
14 checks passed
@yeya24 yeya24 deleted the fix-query-stats-log-qfe branch November 15, 2023 04:23
yeya24 added a commit to yeya24/cortex that referenced this pull request Nov 16, 2023
…ect#5655)

* fix param_query not logged in query stats log

Signed-off-by: Ben Ye <benye@amazon.com>

* fix lint

Signed-off-by: Ben Ye <benye@amazon.com>

* fix unit test of user agent

Signed-off-by: Ben Ye <benye@amazon.com>

---------

Signed-off-by: Ben Ye <benye@amazon.com>
yeya24 added a commit that referenced this pull request Nov 16, 2023
* Fix param_query omitted in query frontend query stats log (#5655)

* fix param_query not logged in query stats log

Signed-off-by: Ben Ye <benye@amazon.com>

* fix lint

Signed-off-by: Ben Ye <benye@amazon.com>

* fix unit test of user agent

Signed-off-by: Ben Ye <benye@amazon.com>

---------

Signed-off-by: Ben Ye <benye@amazon.com>

* Add querier.max-subquery-steps to make subquery step size check optional (#5656)

* add querier.max-subquery-steps to make subquery step size check optional

Signed-off-by: Ben Ye <benye@amazon.com>

* update

Signed-off-by: Ben Ye <benye@amazon.com>

* disable subquery step size check by default, make it optional

Signed-off-by: Ben Ye <benye@amazon.com>

* fix integ test and add changelog

Signed-off-by: Ben Ye <benye@amazon.com>

---------

Signed-off-by: Ben Ye <benye@amazon.com>

* bump RC version to 1.16.0-rc.1

Signed-off-by: Ben Ye <benye@amazon.com>

---------

Signed-off-by: Ben Ye <benye@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants