Add prometheus metrics collectors for http handlers #958

t-tomalak · 2018-12-03T22:15:17Z

What is the problem I am trying to address?

In Athens we don't collect metrics for http requests

How is the fix applied?

Added middleware that wraps buffalo handlers and instruments them to collect count of requests, requests duration, response size, request sizes. As first I wanted to add base http stats in follow up Pull Request there can be added other stats collectors . Some of them are:

Errors - count,
Disk usage
network usage
For workers number of in/out work items and time spent in a queue
Cache hit rate (because it's fun to watch)

Mention the issue number it fixes or add the details of the changes if it doesn't have a specific issue.

Fixes #816

ghost

Just a quick glance so far. But super excited for the possibilities. Like memory and disk, error count 😁

ghost · 2018-12-03T22:40:03Z

cmd/proxy/actions/app.go

-	forcessl "github.com/gobuffalo/mw-forcessl"
-	paramlogger "github.com/gobuffalo/mw-paramlogger"
-	sessions "github.com/gobuffalo/x/sessions"
+	"github.com/gobuffalo/mw-forcessl"


These three imports should not be changed, unless their respective uses are also updated.

I double checked that and I see that they are redundant. github.com/gobuffalo/mw-forcessl is package forcessl and github.com/gobuffalo/mw-paramlogger is package paramlogger. I can revert, but with this changes everything still works 🙂

Oh weird, just the import path doesn't match. I was wondering why the build didn't fail.

manugupt1

Hi @t-tomalak,
Thanks for getting this started. Before digging deep into it, I have a couple of requiests.
Can we move metrics to observ package and use opencensus as it was earlier decided. Further, by using opencensus, we can use various exporters pretty easily?

Would you mind making those changes?

codecov-io · 2018-12-05T21:31:34Z

Codecov Report

Merging #958 into master will increase coverage by <.01%.
The diff coverage is 62.5%.

@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
+ Coverage   59.66%   59.67%   +<.01%     
==========================================
  Files          81       81              
  Lines        2603     2611       +8     
==========================================
+ Hits         1553     1558       +5     
- Misses        908      909       +1     
- Partials      142      144       +2

Impacted Files	Coverage Δ
pkg/config/config.go	`27.27% <ø> (ø)`	⬆️
cmd/proxy/actions/app_proxy.go	`93.75% <100%> (+0.13%)`	⬆️
cmd/proxy/actions/app.go	`50.5% <57.14%> (+0.5%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5eba6f2...d74b16d. Read the comment docs.

michalpristas · 2018-12-06T06:57:43Z

pkg/metrics/http.go

+		prometheus.HistogramOpts{
+			Name:    "athens_http_request_duration_seconds",
+			Help:    "Duration of request handled by Athens server",
+			Buckets: []float64{.1, 1, 5, 10},


this is in seconds right? do you think we can have a .5 as well

Yes this are the second. Sure we can add that. I just put here some sample seconds which were feel right for my local Athens proxy. Any suggestions here are welcome.

michalpristas · 2018-12-06T07:01:19Z

pkg/metrics/http.go

+
+// Middleware is middleware that instruments buffalo handlers
+func Middleware(collectors ...prometheus.Collector) buffalo.MiddlewareFunc {
+	return func(next buffalo.Handler) buffalo.Handler {


trying to figure out if we can be buffalo independent here, but i guess it's ok, we will revisit once we're there

Prometheus accept only http.Handler from std, but unfortunately buffalo handlers are not compatible with it. I know that there are plans to migrate out of buffalo but then it will be just the case of building new middleware.

t-tomalak · 2018-12-06T21:28:33Z

@manugupt1 now metrics are exported using opencensus exporters 🙂

t-tomalak · 2018-12-06T21:33:10Z

pkg/observ/stats.go

+		return nil, errors.E(op, err)
+	}
+
+	return func() {}, nil


Currently func() {} it's not needed but when we would add StackDriver or DataDog exporter it will be needed and by having that already here it will be easy to implement them.

can you then add a comment there so we know why it is there.
i'm not a big fan of future code and what if we will need it approach.

Added, also removed that func() from registerPrometheusExporter method

manugupt1 · 2018-12-08T01:51:21Z

Hi @t-tomalak, codewise it looks perfect to me. This is rad.

Can you link the instructions that you used to run it. So, that I can use it to test it. You can also add it in the docs in this PR if you would like to?

Have a great holiday!

michalpristas · 2018-12-10T08:29:33Z

pkg/observ/stats.go

+}
+
+// registerPrometheusExporter creates exporter that collects stats for Prometheus.
+func registerPrometheusExporter(app *buffalo.App, service string) (func(), error) {


no need to return func(), this is not the iface method, return arg is ignored up there and RegisterStatsExporter returns func(){} by itselft

t-tomalak · 2018-12-11T18:52:35Z

@manugupt1 I already created #970 issue that will allow to automaticaly set up prometheus in local environment, so I don't think we need docs about how to setup prometheus. Metrics can be either checkd manually on localhost:3000/metrics or you can start own prometheus. You can start it by creating configuration file prometheus.yml

global:
  scrape_interval: 2s

  external_labels:
    monitor: 'athens'

scrape_configs:
  - job_name: 'athens'

    scrape_interval: 2s

    static_configs:
      - targets: ['localhost:3000']

Then run:
docker run --name prometheus --network=host -v prometheus.yml:/etc/prometheus/prometheus.yml -d -p 9090:9090 prom/prometheus

And after that prometheus should be accessible under loclhost:9090 in your browser.

manugupt1

This is awesome @t-tomalak!

I am approving it! We will create new issues as they come through.

marpio · 2018-12-18T20:58:47Z

pkg/observ/stats.go

+// RegisterStatsExporter determines the type of StatsExporter service for exporting stats from Opencensus
+// Currently it supports: prometheus
+func RegisterStatsExporter(app *buffalo.App, statsExporter, service string) (func(), error) {
+	const op errors.Op = "RegisterStatsExporter"


Can you add the pkg name like observ.RegisterStatsExporter ?

sure, I was keeping convention from second obervable file with traces. Should I change there also?

I guess yes so I added 🙂

marpio · 2018-12-18T21:00:12Z

pkg/observ/stats.go

+
+// registerViews register stats which should be collected in Athens
+func registerViews() error {
+	const op errors.Op = "registerViews"


add pkg name please

marpio

Thanks Tomasz!

michalpristas · 2018-12-20T07:57:59Z

pkg/observ/stats.go

+}
+
+// StatsMiddleware is middleware that instruments buffalo handlers
+func StatsMiddleware() buffalo.MiddlewareFunc {


just a small nit, can be exported first and unexported after them?

Sorted! Thanks for review 🙂

michalpristas

just a small nit and then 🚢 it

Add prometheus metrics exporter for http handlers

a9e1dc0

t-tomalak requested a review from a team as a code owner December 3, 2018 22:15

ghost reviewed Dec 3, 2018

View reviewed changes

manugupt1 requested changes Dec 4, 2018

View reviewed changes

Merge branch 'master' into 816-http-metrics

a128753

michalpristas reviewed Dec 6, 2018

View reviewed changes

t-tomalak changed the title ~~Add prometheus metrics collectors for http handlers~~ [WIP] Add prometheus metrics collectors for http handlers Dec 6, 2018

Use prometheus from opencensus

7ecbbaa

t-tomalak changed the title ~~[WIP] Add prometheus metrics collectors for http handlers~~ Add prometheus metrics collectors for http handlers Dec 6, 2018

t-tomalak commented Dec 6, 2018

View reviewed changes

manugupt1 and others added 3 commits December 6, 2018 16:49

Merge branch 'master' into 816-http-metrics

df5b1f0

Merge branch 'master' into 816-http-metrics

5096cfd

Merge branch 'master' into 816-http-metrics

e5758cb

michalpristas reviewed Dec 10, 2018

View reviewed changes

Merge branch 'master' into 816-http-metrics

b071687

t-tomalak and others added 3 commits December 11, 2018 20:04

Add comment to unused func() param

f446076

Merge branch 'master' into 816-http-metrics

c1b406b

Merge branch 'master' into 816-http-metrics

7dcbc87

manugupt1 approved these changes Dec 12, 2018

View reviewed changes

t-tomalak and others added 3 commits December 12, 2018 21:58

Merge branch 'master' into 816-http-metrics

aa768fc

Merge branch 'master' into 816-http-metrics

deb9521

Merge branch 'master' into 816-http-metrics

d1eef29

marpio reviewed Dec 18, 2018

View reviewed changes

t-tomalak added 2 commits December 18, 2018 22:18

Add package names to operation description in observ package

af32bd3

Merge branch 'master' into 816-http-metrics

4a4b0cd

marpio approved these changes Dec 20, 2018

View reviewed changes

michalpristas reviewed Dec 20, 2018

View reviewed changes

michalpristas approved these changes Dec 20, 2018

View reviewed changes

Sort exported/unexported functions

d74b16d

michalpristas merged commit fb696b2 into gomods:master Dec 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prometheus metrics collectors for http handlers #958

Add prometheus metrics collectors for http handlers #958

t-tomalak commented Dec 3, 2018 •

edited

Loading

ghost left a comment

ghost Dec 3, 2018

t-tomalak Dec 3, 2018

ghost Dec 5, 2018

manugupt1 left a comment

codecov-io commented Dec 5, 2018 •

edited

Loading

michalpristas Dec 6, 2018

t-tomalak Dec 6, 2018

michalpristas Dec 6, 2018

t-tomalak Dec 6, 2018

t-tomalak commented Dec 6, 2018

t-tomalak Dec 6, 2018 •

edited

Loading

michalpristas Dec 9, 2018

t-tomalak Dec 11, 2018

manugupt1 commented Dec 8, 2018

michalpristas Dec 10, 2018

t-tomalak commented Dec 11, 2018

manugupt1 left a comment

marpio Dec 18, 2018

t-tomalak Dec 18, 2018 •

edited

Loading

t-tomalak Dec 18, 2018

marpio Dec 18, 2018

marpio left a comment

michalpristas Dec 20, 2018

t-tomalak Dec 20, 2018

michalpristas left a comment

Add prometheus metrics collectors for http handlers #958

Add prometheus metrics collectors for http handlers #958

Conversation

t-tomalak commented Dec 3, 2018 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manugupt1 left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 5, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-tomalak commented Dec 6, 2018

t-tomalak Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manugupt1 commented Dec 8, 2018

Choose a reason for hiding this comment

t-tomalak commented Dec 11, 2018

manugupt1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-tomalak Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marpio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalpristas left a comment

Choose a reason for hiding this comment

t-tomalak commented Dec 3, 2018 •

edited

Loading

codecov-io commented Dec 5, 2018 •

edited

Loading

t-tomalak Dec 6, 2018 •

edited

Loading

t-tomalak Dec 18, 2018 •

edited

Loading