Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: use docker/go-metrics utilities for prometheus, add "no_metrics" build-tag #1659

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thaJeztah
Copy link
Contributor

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_metrics build-tag to allow building without metrics.

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 <[email protected]>
@thaJeztah thaJeztah marked this pull request as ready for review November 7, 2022 13:33
@thaJeztah
Copy link
Contributor Author

@jonnystoten @justincormack PTAL - this should get us out of the dependency hell for prometheus, but ⚠️ I'm not a prometheus user; I tried to keep the metrics the same, but please double check if this looks good.

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 no_metrics build-tag which would allow for building without prometheus support, for situations where it's not needed.

Comment on lines +29 to +33
// 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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New situation will have two labels; "old" (operation: <handlerName>) and "new" (handler: <handlerName>)

Comment on lines +23 to +26
if _, registered := instrumented[handlerName]; registered {
// handler for this operation is already registered.
return handler
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thaJeztah
Copy link
Contributor Author

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!)

@brackendawson
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants