server: use docker/go-metrics utilities for prometheus, add "no_metrics" build-tag#1659
server: use docker/go-metrics utilities for prometheus, add "no_metrics" build-tag#1659thaJeztah wants to merge 3 commits intonotaryproject:masterfrom
Conversation
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The old code was no longer compatible with current versions of prometheus. This switches the code to use docker/go-metrics, which is compatible with current versions of prometheus, and already in use in other code in the dependency tree. I tried to keep the metrics the same as before, but there may be some differences. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
@jonnystoten @justincormack PTAL - this should get us out of the dependency hell for prometheus, but Once this is merged, we're able to update dependencies to current versions (and keep it compatible with code using this module, which very likely uses a more current version of prometheus). I added a |
| // Preserve the old situation, which used ConstLabels: "operation: <operation>" | ||
| // for metrics, but ConstLabels in go-metrics are per-namespace, and use | ||
| // ConstLabels: "handler: <handlerName>" (we pass operationName as handlerName). | ||
| namespace := metrics.NewNamespace(namespacePrefix, "http", metrics.Labels{"operation": handlerName}) | ||
| httpMetrics := namespace.NewDefaultHttpMetrics(handlerName) |
There was a problem hiding this comment.
New situation will have two labels; "old" (operation: <handlerName>) and "new" (handler: <handlerName>)
| if _, registered := instrumented[handlerName]; registered { | ||
| // handler for this operation is already registered. | ||
| return handler | ||
| } |
There was a problem hiding this comment.
Had to add this to prevent the handlers.Changefeed being registered twice; it's used for two endpoints, bot using Changefeed as handlerName / operationName. Double check if it still works as expected with this.
|
I see there's an alternative PR already for the metrics in #1570. /cc @brackendawson (not sure which approach works best, but more eyes welcome!) |
|
So both are a breaking change to the metric names. This PR would give notary the same metrics as distribution/distribution right? I could see that being a desirable feature. When I looked at this I tried to find the closest substitute for the discontinued prometheus middleware and went with the examples from the prometheus docs. |
2 participants
The old code was no longer compatible with current versions of prometheus.
This switches the code to use docker/go-metrics, which is compatible with
current versions of prometheus, and already in use in other code in the
dependency tree.
I tried to keep the metrics the same as before, but there may be some
differences.
The first commit separates the metrics code from the server file; second commit introduces a
no_metricsbuild-tag to allow building without metrics.