Add recovery middleware to Ingester; re-add recovery middleware to Querier when not running in standalone mode #4349
Conversation
There was a problem hiding this comment.
Nice catch for #4301. I think Queriers are solved with this but not ingesters.
Ingesters read path is using GRPC so you need an interceptor. I did this a year ago here https://github.com/grafana/loki/blob/main/pkg/util/server/recovery.go#L35 from this PR https://github.com/grafana/loki/pull/2059/files
I think it has been removed by mistake here https://github.com/grafana/loki/pull/4153/files.
cc @owen-d to confirm.
There was a problem hiding this comment.
In #4301 we refactored the Querier but removed the recovery HTTP middleware from the Querier when not running in standalone mode.
I believe this is still handled here in registerRoutesExternally.
Nice catch @cyriltovena! @JordanRushing I think we can reintroduce these two lines to setupAuthMiddleware: https://github.com/grafana/loki/pull/4153/files#diff-f8d1e3ee4777a88e31fef8a16d19b415de6ef999ef259b0dd3dad3e42c6ca015L226-L227
Doing PR review for this one took a lot of file navigation and double-checking, which is probably a good signal that we should refactor this, but not needed now. I've had my eyes on the Cortex api pkg for a while now :)
…erier when not running in standalone mode
JordanRushing
force-pushed
the
jordanrushing/loki/add-recovery-middleware-to-ingester
branch
from
eaaa679
to
3103a19
Compare
…w function setupGRPCRecoveryMiddleware()
What this PR does / why we need it:
In #4301 we refactored the Querier but removed the recovery HTTP middleware from the Querier when not running in
standalonemode.Recently in production we observed an issue (fixed in #4308) where panics occurred in both the Querier and Ingester. The Queriers were able to recover but not the Ingesters.
Which issue(s) this PR fixes:
Fixes #4325.
Special notes for your reviewer:
N/A
Checklist