Loki: fix handling of tail requests when using target all or read
#4642
Conversation
…nning in the same process
slim-bean
force-pushed
the
handle-tail-requests
branch
from
5022f8c
to
5122df0
Compare
pkg/loki/modules.go
Outdated
| @@ -480,7 +493,8 @@ func (t *Loki) initQueryFrontend() (_ services.Service, err error) { | |||
| ).Wrap(frontendHandler) | |||
|
|
|||
| var defaultHandler http.Handler | |||
| if t.Cfg.Frontend.TailProxyURL != "" { | |||
| // If this process also acts as a Querier we don't do any proxying of tail requests | |||
| if t.Cfg.Frontend.TailProxyURL != "" && !t.ModuleManager.IsModuleRegistered(Querier) { | |||
There was a problem hiding this comment.
This isn't actually testing if querier is in the active dependency tree, but only that it's a registered module, which is always the case
There was a problem hiding this comment.
I've traditionally used isModuleEnabled which I agree is a bit hacky since it checks for the string in the target. I think if we looked for the Querier, All, and Read targets we should be covered.
There was a problem hiding this comment.
Great catch, 😠 that I missed this!
There was a problem hiding this comment.
Yeah i went this approach because the isModuleEnabled required supplying multiple targets and I thought this approach would work better...
Instead I've added a method which does exactly what I want
There was a problem hiding this comment.
do we still need both versions of that function?
There was a problem hiding this comment.
I think this new function is a better solution to what we did a lot with isModuleEnabled however I'm a bit scared to make that overhaul today.
I created #4644 to track doing this in the future.
…t supplied to Loki
pkg/loki/loki.go
Outdated
| func (t *Loki) isModuleEnabled(m string) bool { | ||
| for _, tg := range t.Cfg.Target { | ||
| if tg == m { | ||
| return true | ||
| } | ||
| if k, ok := t.deps[tg]; ok { | ||
| for _, dp := range k { | ||
| if dp == m { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
ohh, this is a much better check of dependencies, should we replace the function of the same name on the Config struct with this version and use that everywhere?
There was a problem hiding this comment.
Worried about introducing bugs as we close in on the 2.4 release so I created #4644 to track us doing this in the future.
pkg/loki/loki.go
Outdated
| t.ModuleManager = mm | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (t *Loki) isModuleEnabled(m string) bool { |
There was a problem hiding this comment.
Can we make this isModuleActive? We have another function with the same name on the Config type which behaves differently: https://github.com/grafana/loki/blob/main/pkg/loki/loki.go#L196-L198
There was a problem hiding this comment.
Ideally this would be a recursive check so we could handle IsActive(buzz) == true when target=foo
foo: {bar}
bar: {bazz}
bazz: {buzz}
edit: we should also recontribute that upstream, but I'm fine coding it here for the moment
There was a problem hiding this comment.
Renamed and made it recursive
There was a problem hiding this comment.
Just double checking: there's no chance of Loki having a cyclic dependency, right? Ex:
foo: {bar}
bar: {bazz}
bazz: {bar}Because, if there is, I think it could end in an infinite loop (solution would be to track the visited modules and to not visit already visited nodes).
There was a problem hiding this comment.
haha, i think this could totally happen.
What this PR does / why we need it:
All and Read targets include a frontend which was registering the tail routes, however it currently handles these routes by proxying them to a querier via the
tail_proxy_urlfrontend config. This is difficult and strange behavior when the frontend and querier are in the same process.This PR changes the registration of the tail routes to always be done by the querier and if the query-fronted and querier are in the same process the query-frontend skips registering of the tail routes and the proxy and defers these to be handled by the querier instead.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CHANGELOG.mdabout the changes.