appsflyer / go-sundheit Goto Github PK

This might be a bit of an anti-pattern and a bit of a quirk of our environment rather than something that is widely beneficial. We're currently using this package to provide a health check that is consumed via an ALB within AWS. It works well, but we have a dependancy in there which is degrading (SOLR), not catastrophic. Therefore, I would like the ability to have a healthcheck that doesn't return a 503 if failed, as a soft check. That way, if we are seeing issues, we can still diagnose quickly as we can see the issue from an application's perspective, but we are in a degraded state that is not fully customer impacting.

Right now, a 503 error will cause our instances to restart, causing us to have operational issues for customers instead. A soft failure here would give us the best of both words of visibility but not impacting our state. We would obviously monitor this application via another mechanism.

Is this something I can already achieve? Alternatively, is this something you think makes sense and would be interested in having as part of go-sundheit?

Similar to this, but not necessarily tied to SQL.

Now that OpenTelemetry is becoming a thing, adding support for it would be nice.

However, both OpenTelemetry and OpenCensus are quite heavy dependencies and one might not want to pull in both.

I propose adding an interface for recording metrics that's independent from any metrics collection library. It would allow users to use any implementation (one might just want to use the Prometheus client library, go-kit users could use the go-kit metrics abstraction, etc).

go-sundheit could still ship an OpenCensus implementation, but preferably in a separate module, so that its dependencies are not pulled in by default.

I'd be happy to provide a PR that first relocates OpenCensus integration to a separate module behind a Metrics interface, then an OpenTelemetry implementation in a second PR.

WDYT?

It would be nice if the library offered a way to trigger a function when the service's health status changed. This could be used for logging, or for stopping/starting some components, when the services becomes unhealthy/healthy.

#6 (comment)

Move the code under /health (major release)

Nice little package :-)

In my experience, it's fairly common to expose alongside the health check values some static information about the service, typically the version.

Problem is, the only way to expose those information with go-sundheit is to define a check which means having a ticker refreshing the value. A ticker is not a performance issue but it's still a bit silly. It would be nice if go-sundheit allowed to do that in a less hacky way.

I used example in README like this:

import (
  "bytes"
  "fmt"
  health "github.com/AppsFlyer/go-sundheit"
  //"io/ioutil"
  "net/http"
  "time"
  "log"

  healthhttp "github.com/AppsFlyer/go-sundheit/http"
  "github.com/AppsFlyer/go-sundheit/checks"
)

func main() {
  // create a new health instance
  h := health.New()
  s := '{"test":"json"}'
  r := bytes.NewBuffer([]byte(s))
  httpCheckConf := checks.HTTPCheckConfig{
    CheckName: "reverse",
    Timeout:   1 * time.Second,
    Body:	r,
    // dependency you're checking - use your own URL here...
    // this URL will fail 50% of the times
    URL:       "http://127.0.0.1:9991/g",
  }
  // create the HTTP check for the dependency
  // fail fast when you misconfigured the URL. Don't ignore errors!!!
  httpCheck, err := checks.NewHTTPCheck(httpCheckConf)
  if err != nil {
    fmt.Println(err)
    return // your call...
  }

  // Alternatively panic when creating a check fails
  httpCheck = checks.Must(checks.NewHTTPCheck(httpCheckConf))

  err = h.RegisterCheck(&health.Config{
    Check:           httpCheck, 
    InitialDelay:    time.Second,      // the check will run once after 1 sec
    ExecutionPeriod: 10 * time.Second, // the check will be executed every 10 sec
  })
  
  if err != nil {
    fmt.Println("Failed to register check: ", err)
    return // or whatever
  }

  // define more checks...
  
  // register a health endpoint
  http.Handle("/admin/health.json", healthhttp.HandleHealthJSON(h))
  
  // serve HTTP
  log.Fatal(http.ListenAndServe(":8080", nil))
}

In this code we POST simple JSON
s := '{"test":"json"}' r := bytes.NewBuffer([]byte(s))

When app started everything is OK - request sent as expected. But on the second iteration i have empty Body in request because they have type io.Reader and can`t use second time.
tcpdump:

GET /g HTTP/1.1
Host: 127.0.0.1:9991
User-Agent: Go-http-client/1.1
Content-Length: 15
Accept-Encoding: gzip

{"test":"json"}HTTP/1.1 404 Not Found
Content-Type: text/plain
Date: Wed, 29 Apr 2020 13:46:40 GMT
Content-Length: 18

404 page not found
-----

GET /g HTTP/1.1
Host: 127.0.0.1:9991
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip

HTTP/1.1 404 Not Found
Content-Type: text/plain
Date: Wed, 29 Apr 2020 12:36:04 GMT
Content-Length: 18

404 page not found

I am newby in Golang and maybe don't understand how to send Body on each check.
In my fork i change type of Body to 'string' and write NewRequest via NewReader:

- req, err := http.NewRequest(check.config.Method, check.config.URL, check.config.Body)
+ req, err := http.NewRequest(check.config.Method, check.config.URL, bytes.NewReader([]byte(check.config.Body)))

After that example send Body on each ticker
diff master...revverse:master

While working on the API improvements I found a few Sleeps in the code that caused flaky test runs for me.

I'd suggest adding https://github.com/jonboulle/clockwork to the mix.

Indentation uses 2 spaces instead of tabs

Hey,
is it possible to register different checks / coniditions for the Kubernetes probe types (readines, liveness & startup probe) and expose these different probe types on different endpoints?

For instance I may want to have a single Database check (which checks DB connectivity). If this check fails this should make the readiness & startup probe/endpoint fail but not the liveness probe.

A couple ideas to improve the API of the package:

RegisterCheck should accept a check and a config separately: RegisterCheck(check Check, cfg *Config) error

This would allow making the config optional (given the health checker accepts a default config applied to all checks). I realize this might not make sense in all cases, but it would give the user a cleaner API when they get to learn the package.

I could even imagine

RegisterCheck(check Check) error
RegisterCheckWithConfig(check Check, cfg Config) error

Or with functional options:

type CheckOption interface {
    applyCheck(check *check)
}

RegisterCheck(check Check, ...CheckOption) error

(Note: #25 implements functional options, this would work on top of that)

Consider dropping deregistration: is it really a useful feature? Making the health checker mutable this way is confusing and prone to error in my opinion. Obviously, if there is a legitimate use case it should stay.

As a consequence, I'd also add a Start (and Stop?) method to the health checker and make RegisterCheck return an error for a started health checker.

Alternatively, create a separate HealthBuilder that's mutable and build a HealthChecker with the builder that's immutable.

I realize these are huge changes, but in the long run these could improve the package API by making it more obvious and compact.

Hey,

Very cool package, I'm planning to add it to my toolchain but unfortunately you only have openCensus ("deprecated"). Did you ever though about migrating to OpenTelemetry?

LMK, perhaps I can contribute a new tracer.

Best
Ori

Please add an option to add custom http headers to http checks