-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
querier, receiver, sidecar, store: Add gRPC health check endpoints #2008
Conversation
Another flaky test run. Can someone re-run it? |
@GiedriusS I did another iteration on gRPC health checks. Rather than integrating with prober, I have added grpcHealthServer separately. Not all the components serve gRPC, so they don't use prober. I'm not very sure about API, I'm considering adding additional @squat Also I'd be glad if you can also have a look at this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, looking good. Some suggestions commented on below.
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@bwplotka I did another iteration on it. How does it look? Any suggestions? |
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice I like it 👍
Have one suggestion though.
pkg/prober/grpc.go
Outdated
// Ready sets components status to ready. | ||
func (p *GRPCProbe) Ready() { | ||
p.h.SetServingStatus("", grpc_health.HealthCheckResponse_SERVING) | ||
level.Info(p.logger).Log("msg", "changing probe status", "status", "ready") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, wonder if it's consistent and clean to have log item added to gRPC methods but metrics added to HTTP methods. It's kind of weird. I would either:
- add single logging/metric prober (so prober responsible only to log/metric) and always combine all to the others
- add log line and metric to combine prober and use it everywhere. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll find a way to neatly combine observability layer for prober.
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfecto 👌 Clean and nice, LGTM
Thank you! 😍 |
Please give us feedback if that works for you 💪🤞
…On Thu, 30 Jan 2020, 11:53 Kyrill, ***@***.***> wrote:
Thank you! 😍
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2008?email_source=notifications&email_token=ABVA3O2ZDCMRZQAKZVQS6XLRAK5T3A5CNFSM4KHSFV72YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKKW2HY#issuecomment-580218143>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABVA3O76GB347OC2LN7NZNDRAK5T3ANCNFSM4KHSFV7Q>
.
|
It works perfectly! Thank you 👍 |
This PR adds gRPC Health handlers to gRPC servers.
Attempts to fix #1980.
Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com
Changes
Verification
Tested using https://github.com/grpc-ecosystem/grpc-health-probe
MINIO_ENABLED=1 ./scripts/quickstart.sh