feat: add grpc health, deprecate old health#861
Conversation
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
||
| path, handler := schemaConnectV1.NewServiceHandler(fes, append(svcConf.Options, marshalOpts)...) | ||
| mux.Handle(path, handler) | ||
| mux.Handle(grpchealth.NewHandler(grpchealth.NewStaticChecker( |
There was a problem hiding this comment.
Standard implementation, recommended by buf/conntect.
You can test this locally with grpcurl (below command assumes you've downloaded the health proto to ~/Downloads):
grpcurl -import-path '/home/{your-user}/Downloads/' -proto health.proto -plaintext localhost:8013 grpc.health.v1.Health/Check
| s.server = &http.Server{ | ||
| ReadHeaderTimeout: time.Second, | ||
| Handler: handler, | ||
| Handler: mux, |
There was a problem hiding this comment.
The mux created above was actually unused - instead we just used the one handler originally.
| @@ -206,8 +219,10 @@ func (s *ConnectService) startMetricsServer(svcConf service.Configuration) error | |||
| s.metricsServer.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
| switch r.URL.Path { | |||
There was a problem hiding this comment.
In my new implementation, I've used mux instead of this kind of switch statement.
toddbaert
requested review from
Kavindu-Dodan,
bacherfl,
beeme1mr and
thisthat
toddbaert
force-pushed
the
grpc-healthcheck
branch
2 times, most recently
from
5374cac to
8414c80
Compare
Codecov Report
@@ Coverage Diff @@
## main #861 +/- ##
==========================================
- Coverage 72.67% 72.33% -0.35%
==========================================
Files 27 27
Lines 2759 2772 +13
==========================================
Hits 2005 2005
- Misses 667 678 +11
- Partials 87 89 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| "time" | ||
|
|
||
| schemaConnectV1 "buf.build/gen/go/open-feature/flagd/bufbuild/connect-go/schema/v1/schemav1connect" | ||
| "connectrpc.com/grpchealth" |
There was a problem hiding this comment.
this package is a connect-compatible implementation of a gRPC healthcheck.
* adds a standard gRPC health check * moves HTTP health checks off the metrics port, to the app port * deprecates old health checks on metrics port Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
toddbaert
force-pushed
the
grpc-healthcheck
branch
from
572e216 to
a00ae5a
Compare
thisthat
left a comment
thisthat
left a comment
There was a problem hiding this comment.
We might consider adding a test using https://github.com/grpc-ecosystem/grpc-health-probe to check the gRPC probes are correct. Currently, the ZD tests use the HTTP probes.
| readinessProbe: | ||
| httpGet: | ||
| path: /readyz | ||
| port: 8014 |
There was a problem hiding this comment.
please, adapt also the ZD tests manifest to use the new port :)
There was a problem hiding this comment.
@thisthat flagd-proxy uses the sync package from core, NOT the flag-evaluation package. For this reason, the health-checks of flagd-proxy are not impacted by this change (which may or may not be good, since things will be a bit inconsistent.
Problem is, I'm not 100% sure I can add HTTP 1.1 health-check support to the sync package since it doesn't use connect.
great idea! |
Kavindu-Dodan
left a comment
Kavindu-Dodan
left a comment
There was a problem hiding this comment.
overal looks good 🙌
approving and hope there will be improvements proposed by this comment - #861 (review)
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
|
Closed in favor of #863 |
3 participants
fixes: #831
closed in favor of: #863