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

Upgrade armon/go-metrics to hashicorp/go-metrics #287

Closed
wants to merge 1 commit into from

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Jun 8, 2023

This will require a major version bump in the library as it changes which global metrics handler gets used (due to the module renaming)

@mkeeler mkeeler requested review from a team and jmurret and removed request for a team June 8, 2023 16:53
@mkeeler mkeeler requested review from rboyer and removed request for jmurret and rboyer June 8, 2023 17:05
@aknuds1
Copy link

aknuds1 commented Aug 14, 2023

@mkeeler are you sure a major version bump is required, since according to semver you're free to make breaking changes so long as you're on major version 0?

@mkeeler
Copy link
Member Author

mkeeler commented Aug 14, 2023

@aknuds1 From an exported API perspective nothing would be broken. The main reason I would advocate for a major version bump would be to signal to consumers that they may need to take some action to make things work properly. Because hashicorp/go-metrics and armon/go-metrics look like distinct modules to Go, both could potentially be compiled into one tool. If the application is configuring metrics handlers for armon/go-metrics (which has its own global metrics instance) and memberlist emits metrics to hashicorp/go-metrics, then the memberlist metrics likely would not show up in how the application is recording them (prometheus, dogstatsd etc.)

IMO all this points to some larger changes being needed in these libraries. Instead of using a default global metrics instance the library ought to allow configuring and interface or a set of callbacks to be used when emitting metrics. That way the application consuming the library is in full control of metrics emission.

@aknuds1
Copy link

aknuds1 commented Aug 14, 2023

Instead of using a default global metrics instance the library ought to allow configuring and interface or a set of callbacks to be used when emitting metrics.

That sounds like the right approach to me.

@balena-zh
Copy link

+1

@adamrothman
Copy link

@rboyer @jmurret @huikang @NodyHub can we get some 👀 on this?

@Tochemey
Copy link

Tochemey commented Oct 5, 2024

Any progress on this issue?

@mkeeler
Copy link
Member Author

mkeeler commented Jan 14, 2025

For those watching, we have decided to take a different approach with migration to hashicorp/go-metrics. Changing all the libraries to use pluggable metrics provider interfaces was proving to be difficult to get the interface right for all uses and knowing that the community has been in need of this for so long we chose to take a bit more pragmatic approach. The goal is still to not surprise anyone with where metrics are emitted and to do as much as possible to prevent mismatched dependencies from causing some metrics to land in the armon/go-metrics global handler while others land in the hashicorp/go-metrics global handler.

To that end build tags now control which metrics stack this library and others in the hashicorp namespace use to emit metrics. That will allow application authors to globally opt-in to one module or the other for all hashicorp libraries.

PR #315 was created to make these changes and has been merged and released as v0.5.2.

Documentation for migrating your metrics usage can be found in the README

TLDR: pull in v0.5.2 of memberlist and instrument your build system to set the hashicorpmetrics build tag.

@mkeeler mkeeler closed this Jan 14, 2025
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.

5 participants