Add "metrics" Prometheus endpoint - total requests by type and code. #417
Conversation
| signer, err := signer.New(certCache, key, config.URLSet, rtvCache, certCache.IsHealthy, | ||
| overrideBaseURL, /*requireHeaders=*/!*flagDevelopment, config.ForwardedRequestHeaders) | ||
| overrideBaseURL, signerRequireHeaders, config.ForwardedRequestHeaders) |
There was a problem hiding this comment.
gofmt was breaking formatting here.
There was a problem hiding this comment.
It breaks the inline comment (/requireHeaders=/)
Before gofmt:
signer, err := signer.New(certCache, key, config.URLSet, rtvCache, certCache.IsHealthy,
overrideBaseURL, /*requireHeaders=*/!*flagDevelopment, config.ForwardedRequestHeaders)
After:
signer, err := signer.New(certCache, key, config.URLSet, rtvCache, certCache.IsHealthy,
overrideBaseURL /*requireHeaders=*/, !*flagDevelopment, config.ForwardedRequestHeaders)
There was a problem hiding this comment.
Might be a bug with gofmt. Although https://golang.org/doc/effective_go.html#commentary says that /* */ is mostly reserved for package comments and disabling code. I vote for making gofmt happy.
| errorMsg, errorCode = "500 internal error", http.StatusInternalServerError | ||
| // Validate HTTP method and params, parse params and attach them to req. | ||
| if !allowedMethods[req.Method] { | ||
| errorMsg, errorCode = "405 method not allowed", http.StatusMethodNotAllowed | ||
| } else { |
There was a problem hiding this comment.
Removed this because 1) there's a reference to matchingRule below that would crash if it was nil, so this check is not sufficient to gracefully handle this hypothetical situation 2) unit tests already verify that matchingRule won't be nil 3) an assert would be nice for readability, but it's probably OK to just delete this. I'll add a comment.
…to metr1 Update Travis Go version to latest minor release of Go 1.10. (ampproject#419)
| github.com/prometheus/common v0.6.0/go.mod h1:eBmuwkDJBwy6iBfxCBob6t6dR6ENT/y+J+Zk0j9GMYc= | ||
| github.com/prometheus/procfs v0.0.0-20181005140218-185b4288413d/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk= | ||
| github.com/prometheus/procfs v0.0.0-20190117184657-bf6a532e95b1/go.mod h1:c3At6R/oaqEKCNdg8wHV1ftS6bRYblBhIjjI8uT2IGk= | ||
| github.com/prometheus/procfs v0.0.2/go.mod h1:TjEm7ze935MbeOT/UhFTIMYKhuLP4wbCsTZCD3I8kEA= | ||
| github.com/prometheus/procfs v0.0.3 h1:CTwfnzjQ+8dS6MhHHu4YswVAD99sL2wjPqP+VkURmKE= |
There was a problem hiding this comment.
Out of curiosity, how big is the amppkg binary after these added deps?
There was a problem hiding this comment.
It's 18.02M with these and 16.98M without them.
packager/mux/mux.go
Outdated
| @@ -137,6 +149,8 @@ func (this *mux) ServeHTTP(resp http.ResponseWriter, req *http.Request) { | |||
| path := req.URL.EscapedPath() | |||
|
|
|||
| // Find the first matching routing rule. | |||
| // Note that matchingRule won't be nil because the last rule in routing | |||
| // matrix has an emptry string and therefore matches any path. | |||
There was a problem hiding this comment.
Might be worth codifying this rather than needing a comment:
- Add a
defaultHandler http.Handlerfield to themuxstruct. - Remove the
""entry from the routing matrix. - Convert
return404into anhttp.Handler. - Change the code in here. :)
There was a problem hiding this comment.
Thanks, I agree it's better to codify this. I've added a "defaultRule" field in mux to utilize the "rule" abstraction (e.g. to continue referring to "rule.prometheusLabel").
packager/mux/mux_test.go
Outdated
| // Trigger a 404 attributed to healthz by adding an unexpected suffix to path. | ||
| pkgt.Get(t, mux, expand(`$HOST/healthzSOME_SUFFIX`)) | ||
| expectedMetrics += ` | ||
| total_requests_by_code_and_url{code="404",handler="healthz"} 1 |
There was a problem hiding this comment.
Optionally, break each of these stanzas up into a separate test, where each one starts with a brand new mux and ends with a call to promtest.CollectAndCompare. This has a few benefits:
- It's easier to diagnose failures if they're more limited in scope.
- We won't have any false successes from some earlier action causing a later expectation to be met.
You could use suite to factor out the setup/teardown.
The benefits seem pretty minor in this case, but also the change seems pretty easy, so /shrug. Your call.
There was a problem hiding this comment.
Thanks for the input. I agree with the pros of singling out each scenario.
My intention was to run an "integration" test to check that when being accumulated, different metrics don't interfere with each other.
To achieve both goals I've put all the scenarios in an array and implemented running them both in isolation and altogether.
…mpproject#417) Add "metrics" endpoint. Provide a Prometheus counter of # of request broken down by request type and HTTP response code. This partly implements request ampproject#357. More metrics to be added.
Add "metrics" endpoint. Provide a Prometheus counter of # of request broken down by request type and HTTP response code.
Part 1 of #357.