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

New internal query result payload format: add support for remote rule evaluation path #4331

Merged
merged 20 commits into from
Mar 8, 2023

Conversation

charleskorn
Copy link
Contributor

@charleskorn charleskorn commented Mar 1, 2023

What this PR does

This PR adds support for decoding the new internal query result payload format in the ruler, and encoding it in the query-frontend to send to rulers.

Which issue(s) this PR fixes or relates to

#4104

Checklist

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

return decodeQueryResponse(valTyp, res)
level.Debug(logger).Log("msg", "query expression successfully evaluated", "qs", query, "tm", ts)

contentTypeHeader := getHeader(resp, "Content-Type")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewers: the query-frontend has always set Content-Type to application/json, so checking it like this with no fallback does not introduce any backwards-compatibility issues.

@charleskorn charleskorn changed the base branch from main to sparsehistogram March 1, 2023 04:43
@charleskorn charleskorn force-pushed the charleskorn/new-query-result-format-in-ruler branch 5 times, most recently from 499aa59 to 3f8d9fb Compare March 2, 2023 01:54
@charleskorn charleskorn marked this pull request as ready for review March 2, 2023 02:12
@charleskorn charleskorn requested review from a team as code owners March 2, 2023 02:12
@charleskorn charleskorn force-pushed the charleskorn/new-query-result-format-in-ruler branch from 9046553 to 4e5d9ed Compare March 2, 2023 04:32
@charleskorn charleskorn changed the title New internal query result payload format: add support for decoding response in ruler New internal query result payload format: add support for remote rule evaluation path Mar 2, 2023
@charleskorn charleskorn force-pushed the charleskorn/new-query-result-format-in-ruler branch from 4e5d9ed to f657daa Compare March 2, 2023 04:45
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Good job, LGTM!

pkg/frontend/querymiddleware/codec.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/codec.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/codec_protobuf.go Outdated Show resolved Hide resolved
pkg/frontend/querymiddleware/codec_protobuf.go Outdated Show resolved Hide resolved
pkg/ruler/remotequerier.go Show resolved Hide resolved
pkg/ruler/remotequerier.go Outdated Show resolved Hide resolved
@charleskorn charleskorn force-pushed the charleskorn/new-query-result-format-in-ruler branch from 9311830 to f52aa61 Compare March 3, 2023 00:30
@charleskorn
Copy link
Contributor Author

charleskorn commented Mar 3, 2023

Converting this PR back to a draft until we can merge this directly to main (which will be possible once #4360 and #4377 are merged).

@charleskorn charleskorn marked this pull request as draft March 3, 2023 00:40
@charleskorn charleskorn changed the base branch from sparsehistogram to main March 8, 2023 00:42
@charleskorn charleskorn force-pushed the charleskorn/new-query-result-format-in-ruler branch from 712a9a1 to 285b4b2 Compare March 8, 2023 00:44
@charleskorn charleskorn marked this pull request as ready for review March 8, 2023 01:05
@charleskorn
Copy link
Contributor Author

This PR should be good to go now, and now targets main instead of sparsehistogram.

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks! I just left a nit, but let's address it in a follow up PR so we already merge this one.

@@ -1571,6 +1571,11 @@ query_frontend:
# ruler.query-frontend.grpc-client-config
[grpc_client_config: <grpc_client>]

# (experimental) Format to use when retrieving query results from
# query-frontends. Supported values: json, protobuf
# CLI flag: -ruler.query-frontend.query-result-response-format
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also mention it in the list of experimental features under docs/sources/mimir/operators-guide/configure/about-versioning.md? It's non blocking, you can do it in a follow up PR too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I've created #4429 to do this.

@pracucci pracucci merged commit 13b91e0 into main Mar 8, 2023
@pracucci pracucci deleted the charleskorn/new-query-result-format-in-ruler branch March 8, 2023 17:57
osg-grafana pushed a commit that referenced this pull request Mar 9, 2023
… evaluation path (#4331)

* Push decodeQueryResponse into query.

* Extract createRequest method.

* Extract JSON decoding to its own type.

* Select decoder based on response content type.

* Add configuration flag to enable requesting the new internal query result payload format from query-frontends.

* Send appropriate Accept header based on configured format.

* Add support for decoding responses sent in protobuf format.

* Update changelog.

# Conflicts:
#	CHANGELOG.md

* Fix typo in comment.

* Add support for encoding responses in the protobuf query result payload format in the query-frontend.

* Refactor integration test to run a new ruler instance for each test case.

* Add integration test for use of protobuf format between ruler and query-frontend.

* Add benchmark.

* Implement content negotiation correctly.

* Rename types.

* Address PR feedback: fix typos

Co-authored-by: Marco Pracucci <marco@pracucci.com>

* Address PR feedback: no need to take receiver

* Address PR feedback: take MIME types from decoders.

* Address PR feedback: rename `defaultFormatter`

* Fix issue introduced during rebase on `main`.

---------

Co-authored-by: Marco Pracucci <marco@pracucci.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants