nytimes / gizmo

A Microservice Toolkit from The New York Times
https://open.nytimes.com/introducing-gizmo-aa7ea463b208
Apache License 2.0
3.76k stars 224 forks source link

Remove Deprecated Prometheus - Use OpenCensus Instead #194

Open marwan-at-work opened 5 years ago

marwan-at-work commented 5 years ago

With OpenCensus becoming a default, and since it supports prometheus as well as a few other backends, I think there's no reason from calling the Prometheus client directly in places such as https://github.com/NYTimes/gizmo/blob/master/server/simple_server.go#L103 and https://github.com/NYTimes/gizmo/blob/master/server/simple_server.go#L122 where both function calls are deprecated according to prometheus docs.

Here are a few options:

  1. Let the user just call gcputil.RegisterStackDriver() themselves, so they can be free to choose other backends if they need to.
  2. Let SimpleServer call gcputil.RegisterStackDriver() internally, but we should probably provide a new server.Config option such as SkipMonitoring or EnableMonitoring depending on the kind of default behavior we want yet still letting the user to be able to opt out of it if they have other needs.
  3. We can have the user pass in an opencensus.Exporter interface and just register it with OpenCensus on Init

Personally, I prefer option 3, and 1 the most

jprobinson commented 5 years ago

I'm in for hooking Prometheus into the kit server as well. Following along with my comments in the gcputil branch, I wonder if we can just have that package return the exporters we need to register.

Internally, I wonder if that new package could be something like observe.NewOCExporter(...) and it would scan the environment (or some simple config?) to determine which exporters to initiate and respond with them. This could check for Prom configs, init that if they exist then fall back to checking GCP creds/Stackdriver options.

I like the idea of (3) too, though: Allow users to pass a custom exporter on their own. I just wonder if thats a use case we need.