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 content negotiation support and sharding parameters for Query Frontend #375

Merged
merged 22 commits into from
Dec 7, 2020

Conversation

annanay25
Copy link
Contributor

@annanay25 annanay25 commented Nov 25, 2020

What this PR does:
This PR is the first half the work to introduce a Query Frontend (follow up of #362), and does a bunch of things:

  • Add parameters to HTTP endpoint of the Querier, which will help with sharding. List of parameters -
    • BlockStart & BlockEnd - to specify the shard space for each querier
    • QueryIngesters - to specify whether a querier should query ingesters (only one of the queriers should query ingesters for every query)
  • Add content negotiation between Tempo-query and Tempo-querier to marshal trace data into proto (and save a roundtrip b/n proto & json)

Which issue(s) this PR fixes:
Fixes #NA

Checklist

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

Makefile Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
@joe-elliott
Copy link
Member

joe-elliott commented Nov 25, 2020

So there's like 3 things going on here

  1. Additional parameters on the query endpoint
  2. Content negotiation to proto for the HTTP endpoint
  3. GRPC for the querier

Given that we know the query frontend is going to want to use HTTP I think it makes sense to drop 3 and only do 1 and 2. Thoughts?

@annanay25
Copy link
Contributor Author

annanay25 commented Nov 25, 2020

Given that we know the query frontend is going to want to use HTTP I think it makes sense to drop 3 and only do 1 and 2. Thoughts?

Hmm, Frontend is going to roundtrip GRPC to the Queriers no? It's going to reuse this - https://github.com/cortexproject/cortex/blob/master/pkg/frontend/v2/frontendv2pb/frontend.proto

Content negotiation to proto for the HTTP endpoint

This should be dropped. Was added in the PR by mistake. I don't think we need that anymore.

@joe-elliott
Copy link
Member

I don't think the query-frontend supports GRPC. Isn't it all built around that http wrapped in grpc trick? Don't we need http on the queriers to use it without major changes?

@annanay25
Copy link
Contributor Author

We can add a GRPC handler to the query frontend. QF <> Querier will be GRPC bi-directional stream. Just the payload is HTTP, but we can marshal proto into the body like we're doing between QF and Jaeger-GRPC plugin.

@annanay25 annanay25 changed the title Use GRPC between Jaeger Query and Tempo Querier Add content negotiation between Tempo-query and Querier, add new parameters in prep for Query Frontend Dec 2, 2020
@annanay25 annanay25 changed the title Add content negotiation between Tempo-query and Querier, add new parameters in prep for Query Frontend Add content negotiation support and sharding parameters for Query Frontend Dec 2, 2020
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Some clean up requested. This is definitely on the right path!

cmd/tempo-query/tempo/plugin.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
modules/querier/http.go Show resolved Hide resolved
pkg/tempopb/tempo.pb.go Outdated Show resolved Hide resolved
tempodb/tempodb.go Outdated Show resolved Hide resolved
tempodb/tempodb.go Outdated Show resolved Hide resolved
Signed-off-by: Annanay <[email protected]>
Signed-off-by: Annanay <[email protected]>
Signed-off-by: Annanay <[email protected]>
Signed-off-by: Annanay <[email protected]>
Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

A few small changes. Looking very good.

tempodb/tempodb.go Outdated Show resolved Hide resolved
pkg/util/http.go Outdated Show resolved Hide resolved
modules/querier/http.go Outdated Show resolved Hide resolved
Signed-off-by: Annanay <[email protected]>
@annanay25
Copy link
Contributor Author

Addressed comments. @joe-elliott - This should be ready for review.

@annanay25 annanay25 merged commit 3710d94 into grafana:master Dec 7, 2020
@annanay25 annanay25 mentioned this pull request Dec 8, 2020
3 tasks
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