chore: `armon/go-metrics => hashicorp/go-metrics`

[peteski22]

This PR is intended to remove the direct dependency within Vault on armon/go-metrics in favor of hashicorp/go-metrics. This should make life easier when we want to update versions going foward.

In addition it also bumps the version to 0.5.3 based on https://github.com/hashicorp/go-metrics/pull/146.

Using go mod graph | grep ' github.com/armon/go-metrics@v\d.\d.\d$' we can identify indirect dependencies on armon/go-metrics in other packages used by Vault, which may require their own PRs if they decide to move away from the library. It would be worth checking each package repo to see if they have tagged newer versions which have already moved to hashicorp/go-metrics and then update Vault to reference them.

github.com/hashicorp/vault github.com/armon/[email protected]
github.com/google/tink/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/consul/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]
github.com/hashicorp/[email protected] github.com/armon/[email protected]

[github-actions[bot]]

CI Results: All Go tests succeeded! :white_check_mark:

[peteski22]

Great job Pete!

Thinking this through... I'm pretty sure this is going to break a lot of things related to metrics due to the (unfortunate IMO) design choices in go-metrics.

The fact that metrics uses global state to setup sinks and instrumentation means that if any libraries are still using the armon version, those metrics are suddenly going to dissapear from Vault's actual metrics collection which will now be using hashicorp right?

At first I thought we'd need to go and fix every one of those deps you listed before we can merge, but actually I think it would be sufficient to also add a

replace github.com/armon/go-metrics => github.com/hashicorp/go-metrics 

Bit we'd need to verify this - it would be great to build before and after this PR and verify that all those library's metrics (e.g. Raft metrics) are still present! It's complicated further by the fact that only metrics reported recently are actually output (anther design issues 😢 ) so it's not trivial to just start vault and hit the metrics endpoint once - we may need to actually ensure things happen to see the metrics. The good news is that as long as we have one metric from each library in your list (that currently outputs metrics) that's OK we don't need to figure out how to trigger every single metric to be output!

Blimey that was fast. 😄

Would the problem of global state not have existed previously since we had the indirect reference to hashicorp/go-metrics (so something down the stack was using it)? 🤔

~Good shout on adding the replace, I'll push that up now.~

Sad about the amount of testing we'd require to be OK with this though, eeek.

[github-actions[bot]]

Build Results: All builds succeeded! :white_check_mark:

[biazmoreira]

Closing https://github.com/hashicorp/vault/pull/21130

[biazmoreira]

@peteski22

It's complicated further by the fact that only metrics reported recently are actually output (anther design issues 😢 ) so it's not trivial to just start vault and hit the metrics endpoint once - we may need to actually ensure things happen to see the metrics.

One idea is to use an HCPV cluster with a custom binary to test this out. There's an amount of work required to enable the observability provider and so on, but not sure how you plan on testing this. In any case, I'm excited to see this finally being done!

[banks]

Would the problem of global state not have existed previously...

I've realised I'm probably wrong here since we actually renamed the repo anyway. I was thinking theyd import as differnt modules but actually since we renamed/redirected and updated to go.mod, I think both paths will end up importing the same module under the declared name hashicorp/go-metrics.

But yeah we should double check that - e.g. that library metrics are still being emitted. If they are then I think this is fine and we don't need a huge effort to check every single library since they will all work the same way!

That probable means the replace directive is unnecessary too?

Sorry for the noise!