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

Add gRPC support to query-tee #2683

Merged
merged 21 commits into from
Aug 18, 2022
Merged

Add gRPC support to query-tee #2683

merged 21 commits into from
Aug 18, 2022

Conversation

ssncferreira
Copy link
Contributor

@ssncferreira ssncferreira commented Aug 10, 2022

What this PR does

Add gRPC support to query-tee. This allows testing rule evaluations of rulers configured with remote operational mode via query-tee.
Query-tee now uses the weaveworks server implementation which creates 2 listening ports:

  • HTTP
  • gRPC (for HTTP over gRPC messages)

To test ruler evaluations with query-tee, update the ruler configuration query-frontend.address configuration to use query-tee's gRPC address:

ruler:
  query_frontend:
    address: "dns://query-tee-addr:9095"

When the ruler evaluates a rule, the flow is the following:

  1. Send gRPC request to query-tee
  2. query-tee forwards the request to the configured ruler-query-frontends of the Mimir instances
  3. query-tee receives the responses from the Mimir instances and forwards the result (based on the preferred backend) to the ruler

Used for testing split by range epic: https://github.com/grafana/mimir-squad/issues/774

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

@ssncferreira

This comment was marked as resolved.

@github-actions

This comment has been minimized.

tools/querytee/proxy.go Outdated Show resolved Hide resolved
tools/querytee/proxy_test.go Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

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.

A couple of more things:

  1. Please add a CHANGELOG entry
  2. Can you add a phrase to docs/sources/operators-guide/tools/query-tee.md under "API endpoints", saying that the query-tee accept both HTTP requests and "HTTP over gRPC" requests, linking HTTP over gRPC to the library?

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

This comment has been minimized.

* Add weaveworks server configuration parameters
* Use common metrics namespace
* Update proxy tests to use port 0
* Update CHANGELOG
* Update query-tee operator's guide documentation
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ssncferreira ssncferreira marked this pull request as ready for review August 12, 2022 09:29
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

CHANGELOG.md Outdated Show resolved Hide resolved
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!

docs/sources/operators-guide/tools/query-tee.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/tools/query-tee.md Outdated Show resolved Hide resolved
tools/querytee/proxy.go Outdated Show resolved Hide resolved
tools/querytee/proxy.go Outdated Show resolved Hide resolved
ssncferreira and others added 8 commits August 16, 2022 14:43
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
* Rename `server.service-port` to `server.http-service-port`
* Rename configuration `ServerServicePort` to `ServerHTTPServicePort`
* Add changelog entry for `server.http-service-port` and `cortex_querytee_backend_request_duration_seconds`
* Update operators-guide/query-tee documentation
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
docs/sources/operators-guide/tools/query-tee.md Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

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.

LGTM!

Co-authored-by: Marco Pracucci <marco@pracucci.com>
@github-actions
Copy link
Contributor

Helm <> Jsonnet Diff

⚠️ A difference was detected between the Helm chart and the Jsonnet library.

  1. Use make check-helm-jsonnet-diff to reproduce the output locally.
  2. This test is experimental while we gather feedback about its usefulness.
  3. It does not block your PR from being merged, but we would appreciate you trying to keep feature parity between the Helm chart and Jsonnet library if possible.

If you get stuck on this step and the Mimir maintainers aren't able to help, feel free to merge without making this step pass and tag @Logiraptor so the Mimir maintainers can gather feedback later.

Please see the contribution docs here for more info.

Expand to see the output

Output of https://github.com/grafana/mimir/actions/runs/2875356531

Warning: policy/v1beta1 PodDisruptionBudget is deprecated in v1.21+, unavailable in v1.25+; use policy/v1 PodDisruptionBudget
diff -r -u -N scratch/./helm/07-config/query-scheduler-MimirConfig.yml scratch/./jsonnet/08-config/query-scheduler-MimirConfig.yml
--- scratch/./helm/07-config/query-scheduler-MimirConfig.yml	2022-08-17 12:40:09.417323891 +0000
+++ scratch/./jsonnet/08-config/query-scheduler-MimirConfig.yml	2022-08-17 12:40:17.601479854 +0000
@@ -498,7 +498,7 @@
       min_ready_duration: 15s (default)
       observe_period: 0s (default)
       replication_factor: 3 (default)
-      tokens_file_path: /data/tokens
+      tokens_file_path: ' (default)'
       zone_awareness_enabled: false (default)
     tsdb_config_update_period: 15s (default)
   ingester_client:
@@ -881,7 +881,7 @@
           secondary: ' (default)'
         store: memberlist (default)
       replication_factor: 3 (default)
-      tokens_file_path: /data/tokens
+      tokens_file_path: ' (default)'
       unregister_on_shutdown: true (default)
       wait_stability_max_duration: 5m0s (default)
       zone_awareness_enabled: false (default)

@pracucci
Copy link
Collaborator

I've just manually tested this PR and looks working great!

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.

None yet

2 participants