frontend: [Refactor] Isolated middlewares from frontend to allow usage from external projects. #1332
Conversation
…e from external projects. No logic should be changed, just refactor that allows middlewares reuse. Changes: * frontend.proto only specifies gRPC service * Middlewares with query range moves to queryrange package * queryrange.proto has rest (query API for range) and is in queryrange package. * Removed util.Logger frmo queryrange * Removed usages of overrides, moved to Limits interface in queryrange. * Ran goimports. * Some namings. * QueryRange proto generated is not commited. I know it might be non consistent, but no easy way for external project to regenerate it (?). * Removed log messages there were duplicated with return error (bad error handling). * Added TODOs * ResultCache Middleware has now config independent constructor. Question: Why all of those methods `get`, `put`, `Fetch` does not return error? TODO in followng PRs: Retry middleware & other TODOs Signed-off-by: Bartek Plotka <[email protected]>
bwplotka
changed the title
|
Will fix build errors later on |
|
Hi @bwplotka! Am on the road right now will give this a thorough look tomorrow morning on the next flight. Cheers! |
| [[override]] | ||
| name = "github.com/prometheus/client_golang" | ||
| branch = "master" | ||
| version = "v0.9.2" |
There was a problem hiding this comment.
Please add a comment giving the reason we need an override, any background why this specific version, and the conditions under which the override can be removed.
There was a problem hiding this comment.
Well, there was comment before (TODO: Pin this version), so I just fullfillted TODO, but sure, can add more info.
There was a problem hiding this comment.
Where do you see "TODO"? I see "Pin to master branch until there is a more recent stable release"
Can the override be removed entirely now?
There was a problem hiding this comment.
? Isn't that TODO-like comment? Didn't I do exactly what is requested as stable release is there now? 😕
"Pin to master branch until there is a more recent stable release"
Anyway, override is required due to jaeger contstraint. Commented all and added in another PR:
#1353
One it will be merged, I will remove this change from this PR.
| @@ -0,0 +1,2232 @@ | |||
| // Code generated by protoc-gen-gogo. DO NOT EDIT. | |||
There was a problem hiding this comment.
We haven't traditionally checked in the .pb.go files.
I realise this particular one may be important to your objectives, but if we do one we should do all.
There was a problem hiding this comment.
Agree, let's discuss.
Did not try to see how painful it will be for other projects to use it without it being not commited, so need to check that first. I assume I would need to understand the gogo version of cortex and regenerate on thanos side - not fun.
There was a problem hiding this comment.
Will try to cover this at the community meeting today.
There was a problem hiding this comment.
My bad - community meeting is tomorrow (2nd May)
There was a problem hiding this comment.
From the community meeting we decided that we would generally like to check in the code with a couple caveats: #1371
I would say this PR should either not check in the pb.go or wait until after the tasks in that issue are completed. I don't want to be in a state where some files are checked in and others are not.
| @@ -764,18 +764,19 @@ | |||
| revision = "fb713f6d8239b57c646cae30f78e8b4b8861a1aa" | |||
|
|
|||
| [[projects]] | |||
| branch = "master" | |||
| digest = "1:511d8b5b4cef21fdc5c660e5d9c46a2d6d039809d9600ff1518b033e300522b9" | |||
| digest = "1:4a8dc3e9c1bb30298bfee9d4b89d4b527410582901bc409a26d11fa1d471ec14" | |||
There was a problem hiding this comment.
Could you split the commit into one which purely updates vendor, and one which does the Cortex changes.
|
Clearly lots of stuff changed so need to rebase (: Currently working on streaming remote read, will get back to caching on later date. Thanks for guidance so far! |
Hello!
This work is done as part of: thanos-io/thanos#1006
PTAL @tomwilkie and thanks for guidance so far! 🥇
No logic should be changed, just refactor that allows middlewares reuse.
Feedback welcome, especially on TODOs I added, if the direction is OK.
FYI: Thanos side changes (very WIP): thanos-io/thanos#1039
Roughly, changes (sorry for biggy PR):
for external project to regenerate it (?).
TODO in followng PRs: Retry middleware & other TODOs
Signed-off-by: Bartek Plotka [email protected]