refactor Prometheus metrics. Less metrics, more labels #384
Conversation
httpapi/metrics.go
Outdated
| @@ -7,20 +7,10 @@ import ( | |||
| func init() { | |||
| prometheus.MustRegister(metricFunctionRegistered) | |||
| prometheus.MustRegister(metricFunctionDeleted) | |||
There was a problem hiding this comment.
@mthenw Would it be better to have a single metricFunctions that is just a gauge of registered functions for a Space?
httpapi/metrics.go
Outdated
| prometheus.MustRegister(metricFunctionDeleteRequests) | ||
| prometheus.MustRegister(metricFunctionUpdateRequests) | ||
| prometheus.MustRegister(metricFunctionListRequests) | ||
|
|
||
| prometheus.MustRegister(metricSubscriptionCreated) | ||
| prometheus.MustRegister(metricSubscriptionDeleted) |
There was a problem hiding this comment.
Same question as above, but with metricSubscriptions
httpapi/httpapi.go
Outdated
| @@ -62,7 +62,7 @@ func (h HTTPAPI) getFunction(w http.ResponseWriter, r *http.Request, params http | |||
| encoder.Encode(fn) | |||
| } | |||
|
|
|||
| metricFunctionGetRequests.WithLabelValues(space).Inc() | |||
| metricConfigRequests.WithLabelValues(space, "function", "get").Inc() | |||
| } | |||
|
|
|||
| func (h HTTPAPI) getFunctions(w http.ResponseWriter, r *http.Request, params httprouter.Params) { | |||
There was a problem hiding this comment.
Small nit, but is it worth making these handler names line up with the labels put in the metrics? listFunctions, createFunction, etc.
| @@ -59,7 +59,7 @@ func (router *Router) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| // isHTTPEvent checks if a request carries HTTP event. It also accepts pre-flight CORS requests because CORS is | |||
| // resolved downstream. | |||
| if isHTTPEvent(r) { | |||
| metricEventsHTTPReceived.Inc() | |||
| metricEventsReceived.WithLabelValues("", "http").Inc() | |||
|
|
|||
| event, _, err := router.eventFromRequest(r) | |||
| if err != nil { | |||
There was a problem hiding this comment.
Would it be easier to put metricEventsProcessed.WithLabelValues(space, "http").Inc() after line 72 below, rather than in the handleHTTPEvent method? Follows the pattern of how the invoke event is done, plus it keeps it together a little better.
Wouldn't let me comment on line 72 directly since it wasn't in the diff.
There was a problem hiding this comment.
Yeah, that would make sense but I don't want to return space from handleHTTPEvent. There is some refactoring needed here. Metrics reporting could also be moved to inside those handle* functions but again there will be exception for HTTP event. Let's leave it as it is right now as there is no ideal solution.
No description provided.