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 hashicorp/go-metrics#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]

[peteski22]

Version bump: 0.5.1 (indirect) => 0.5.3 (direct)

[peteski22]

Version bump: 0.2.0 => 0.6.0

[peteski22]

Using v2 now.

[peteski22]

Moved SDK dependency armon/go-metrics to hashicorp/go-metrics and bumped 0.4.1 => 0.5.3

[github-actions]

CI Results:
All Go tests succeeded! ✅

[peteski22]

hashicorp/go-metrics#151

[peteski22]

hashicorp/go-metrics#151

[banks]

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!

[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]

Build Results:
All builds succeeded! ✅

[biazmoreira]

Closing #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!

[banks]

I've realised I'm probably wrong

Wrong again!

Actually this was a legitimate concern that we have now confirmed and that Nomad also discovered independently.

The issue is that despite the redirect which makes it appear to work fine if you mix armon/ and hashicorp/ imports in a greenfield project, all our products (and other projects no doubt that use our libraries) are currently pinned to a version of go-metrics from before 0.5.0 where the module name was changed.

That means that although the import redirects to hashicorp/ to fetch the code, it's checking out a version of the module whose go.mod still calls itself armon/go-metrics.

That results in Consul, Vault and Nomad setting up and configuring metrics syncs on the armon flavour global metrics instance, while any dependencies that happen to import hashicorp/go-metrics would likely end up on a newer version that calls itslef hashicorp/go-metrics and so their metrics will be reported to a different singleton instance than the one that we are actually outputting from and their metrics will be lost!

Conversely, if any of our products change imports and/or upgrade go-metrics to 0.5.0 or higher, we will loose all the metrics being reported to the older armon/go-metrics import.

There may be some way out of this cleanly but a few folks have looked and it's seeming like a v2 of all our libs will be needed to make the breaking change necessary potentially.

So much as I'd love to resolve this.... this PR should be held back until we work out the strategy across all our products!