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 latest mimir-prometheus #6312

Merged
merged 3 commits into from
Oct 10, 2023
Merged

Conversation

aknuds1
Copy link
Contributor

@aknuds1 aknuds1 commented Oct 10, 2023

What this PR does

Upgrade to latest mimir-prometheus, see PR for its latest sync with upstream. I'm not aware of any behavioral changes that'll affect Mimir.

Please note that I'm replacing github.com/charleskorn/goautoneg with a Grafana Labs fork github.com/grafana/goautoneg, based off of the same commit. I needed to make a fix to goautoneg so it would compile with the new revision of golang.org/x/exp. I think it's better going forward to use a Grafana Labs fork rather than a personal one?

Which issue(s) this PR fixes or relates to

Checklist

  • Tests updated
  • [na] Documentation added
  • [na] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 force-pushed the arve/upgrade-mimir-prometheus branch from 44486a7 to 5738f95 Compare October 10, 2023 12:14
@aknuds1 aknuds1 changed the title WIP: Upgrade to latest mimir-prometheus Upgrade to latest mimir-prometheus Oct 10, 2023
@aknuds1 aknuds1 marked this pull request as ready for review October 10, 2023 12:14
@aknuds1 aknuds1 requested review from grafanabot and a team as code owners October 10, 2023 12:14
@pracucci pracucci self-requested a review October 10, 2023 12:16
if a.Name != b.Name {
return a.Name < b.Name
slices.SortFunc(keys, func(a, b labels.Label) int {
nameCompare := strings.Compare(a.Name, b.Name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be great to open an upstream PR to not use strings.Compare() here. This should be something like:

if a.Name == b.Name {
  // ... compare using < > here ...
}

@@ -412,7 +413,7 @@ func yoloBytes(s string) (b []byte) {
// New returns a sorted Labels from the given labels.
// The caller has to guarantee that all label names are unique.
func New(ls ...Label) Labels {
slices.SortFunc(ls, func(a, b Label) bool { return a.Name < b.Name })
slices.SortFunc(ls, func(a, b Label) int { return strings.Compare(a.Name, b.Name) })
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend to consider replacing strings.Compare() in this file too, and run a benchmark to see the actual perf impact.

@pracucci
Copy link
Collaborator

Comments on vendored Prometheus are not blockers to merge, but please consider it.

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
@aknuds1 aknuds1 requested a review from pracucci October 10, 2023 13:02
Copy link
Collaborator

@pracucci pracucci 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!

@aknuds1
Copy link
Contributor Author

aknuds1 commented Oct 10, 2023

Please note that I'm replacing github.com/charleskorn/goautoneg with a Grafana Labs fork github.com/grafana/goautoneg, based off of the same commit. I needed to make a fix to goautoneg so it would compile with the new revision of golang.org/x/exp. I think it's better going forward to use a Grafana Labs fork rather than a personal one?

cc @charleskorn

@aknuds1 aknuds1 merged commit 8a39aa1 into main Oct 10, 2023
28 checks passed
@aknuds1 aknuds1 deleted the arve/upgrade-mimir-prometheus branch October 10, 2023 13:25
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