Separate HTTP Instrumentation for Querier #2708
Conversation
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
pkg/api/api.go
Outdated
| // Ensure the encoded path is used. Required for the rules API | ||
| s.HTTP.UseEncodedPath() | ||
|
|
||
| // Prometheus histograms for requests. | ||
| querierRequestDuration := prometheus.NewHistogramVec(prometheus.HistogramOpts{ |
There was a problem hiding this comment.
You can simply use promauto.With(reg).NewHistogramVec().
pkg/api/api.go
Outdated
| // RegisterQueryAPI registers the Prometheus routes supported by the | ||
| // Cortex querier service. Currently this can not be registered simultaneously | ||
| // with the Querier. | ||
| func (a *API) RegisterQueryAPI(handler http.Handler) { |
There was a problem hiding this comment.
Is this refactoring related to the scope of the PR? I'm not against it if you need it in a project vendoring Cortex but, if it's not required, probably the previous version was more clear.
There was a problem hiding this comment.
Could you comment on this? I'm fine if you need it in a project vendoring Cortex, otherwise the previous version looks more explicit to me.
pkg/api/api.go
Outdated
| // Ensure the encoded path is used. Required for the rules API | ||
| s.HTTP.UseEncodedPath() | ||
|
|
||
| // Prometheus histograms for requests. | ||
| querierRequestDuration := prometheus.NewHistogramVec(prometheus.HistogramOpts{ |
There was a problem hiding this comment.
Have you considered moving this to RegisterQuerier()? The metric would be registered only if RegisterQuerier() is used and the change isolated there.
There was a problem hiding this comment.
I wanted to avoid passing a prometheus.Registerer to that function. However, maybe it would make more sense to store the Registerer in the API struct and and initiate the metric is in the RegisterQuerier function.
There was a problem hiding this comment.
Yes, or you can pass the metric itself to the RegisterQuerier() (ie. in Thanos it's quite a common design pattern).
There was a problem hiding this comment.
👍 Cool I updated it to pass a metric as a parameter.
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
pkg/api/api.go
Outdated
| // RegisterQueryAPI registers the Prometheus routes supported by the | ||
| // Cortex querier service. Currently this can not be registered simultaneously | ||
| // with the Querier. | ||
| func (a *API) RegisterQueryAPI(handler http.Handler) { |
There was a problem hiding this comment.
Could you comment on this? I'm fine if you need it in a project vendoring Cortex, otherwise the previous version looks more explicit to me.
|
I don't have any particular use case for that function. It came about when I was expirementing with alternative ways of solving the "separate querier instrumentation" problem. I kept it because I thought it looked cleaner. However, I can return it to it's previous form if you fell otherwise. |
|
Up to your judgement, but if we keep it we could make it unexported so that
we signal it's just used internally.
…On Tue, Jun 16, 2020, 19:07 Jacob Lisi ***@***.***> wrote:
#2708 (comment)
<#2708 (comment)>
I don't have any particular use case for that function. It came about when
I was expirementing with alternative ways of solving the "separate querier
instrumentation" problem. I kept it because I thought it looked cleaner.
However, I can return it to it's previous form if you fell otherwise.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2708 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM7QEB2KMZRXQ7YY3DBAK3RW6RFHANCNFSM4N2R3UOQ>
.
|
Signed-off-by: Jacob Lisi <[email protected]>
What this PR does:
When running cortex as a single binary the requests processed by the querier are not instrumented. This PR adds a separate histogram metric
cortex_querier_request_duration_secondsthat allows the operator to make this distinction.The metric is registered in both single binary and microservice mode. This will allow the new metric to be used for dashboards/panels in both modes.
Which issue(s) this PR fixes:
Fixes #2707
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]