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 to using hashicorp/[email protected] #24856

Merged
merged 5 commits into from
Jan 31, 2025
Merged

Upgrade to using hashicorp/[email protected] #24856

merged 5 commits into from
Jan 31, 2025

Conversation

mkeeler
Copy link
Member

@mkeeler mkeeler commented Jan 14, 2025

Description

On the surface this is a fairly mechanical change in deps and imports. Instead of consuming armon/go-metrics, Nomad will now use the latest version of hashicorp/go-metrics. Previous attempts to upgrade resulted in missing metrics and were reverted. This time is different because all libraries within the hashicorp namespace have been instrumented to use a special compat package within hashicorp/go-metrics to allow for a compile time build tag to control the metrics module used by all libraries.

This PR does 3 things:

  • Updates dependencies of the relevant libraries (memberlist, serf, raft, raft-boltdb) to ensure they can support the build tag.
  • Changes all imports of armon/go-metrics to hashicorp/go-metrics
  • Modifies the GO_TAGS variable handling within the makefile to use the hashicorpmetrics build tag (global library opt-in to metrics emission via the hashicorp/go-metrics module)

Testing & Reproduction steps

  • make dev
  • mkdir data
  • nomad agent -server -data-dir $(pwd)/data
  • curl localhost:4646/v1/metrics

Now ensure that metrics from all the libraries are still seen. I checked that there was at least one from serf, memberlist, raft and raft-boltdb.

Contributor Checklist

  • Changelog Entry If this PR changes user-facing behavior, please generate and add a
    • No user facing behavioral changes.
      changelog entry using the make cl command.
  • Testing Please add tests to cover any new functionality or to demonstrate bug fixes and ensure regressions will be caught.
    • No functional changes to test.
  • Documentation If the change impacts user-facing functionality such as the CLI, API, UI,
    and job configuration, please update the Nomad website documentation to reflect this. Refer to
    the website README for docs guidelines. Please also consider whether the
    change requires notes within the upgrade guide.
    • No user facing changes to document.

Reviewer Checklist

  • Backport Labels Please add the correct backport labels as described by the internal
    backporting document.
  • Commit Type Ensure the correct merge method is selected which should be "squash and merge"
    in the majority of situations. The main exceptions are long-lived feature branches or merges where
    history should be preserved.
  • Enterprise PRs If this is an enterprise only PR, please add any required changelog entry
    within the public repository.

@mkeeler mkeeler requested review from a team as code owners January 14, 2025 15:41
@schmichael schmichael self-assigned this Jan 21, 2025
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

LGTM, just want to check a couple points before approving.

GNUmakefile Show resolved Hide resolved
go.mod Show resolved Hide resolved
@@ -5,7 +5,6 @@ go 1.23
// Pinned dependencies are noted in github.com/hashicorp/nomad/issues/11826.
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that our reason for pinning a specific version of armon/go-metrics appears to have been resolved years ago: hashicorp/go-metrics#146

So dropping this pinning is not only ok, but a great side effect.

This also requires bumping the dependencies for:

* memberlist
* serf
* raft
* raft-boltdb
* (and indirectly hashicorp/mdns due to the memberlist or serf update)

Unlike some other HashiCorp products, Nomads root module is currently expected to be consumed by others. This means that it needs to be treated more like our libraries and upgrade to hashicorp/go-metrics by utilizing its compat packages. This allows those importing the root module to control the metrics module used via build tags.
@mkeeler mkeeler requested a review from schmichael January 30, 2025 15:31
@mkeeler
Copy link
Member Author

mkeeler commented Jan 30, 2025

@schmichael I think this is ready for a final look. I migrated the Nomad root module to utilizing the compat package to ensure all those importing that module can control which metric module is used.

@mkeeler
Copy link
Member Author

mkeeler commented Jan 30, 2025

Also while Consul's API package has a memberlist/serf dependency, I have not updated that module in this PR because the equivalent Consul PRs haven't landed yet. This should be perfectly fine because Consul's API package doesn't emit metrics and only uses those dependencies for Coordinate structures types (which Nomad doesn't consume anyways).

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks so much for taking this on @mkeeler !

@mkeeler mkeeler merged commit 833e240 into main Jan 31, 2025
30 checks passed
@mkeeler mkeeler deleted the hashicorp-metrics branch January 31, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

2 participants