-
Notifications
You must be signed in to change notification settings - Fork 68
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 loki and related dependencies #277
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, a HUGE thanks for getting this done @shantanualsi! This was a massive effort.
LGTM, except there are a couple changes which I don't understand.
If you do make any changes, please remember to not force-push; this makes incremental reviews impossible, and for huge PRs like this it's quite important
@@ -55,7 +55,7 @@ func NewRingChecker(id string, instanceName string, cfg RingCheckConfig, workloa | |||
workload: workload, | |||
} | |||
reg := prometheus.DefaultRegisterer | |||
cfg.MemberlistKV.MetricsRegisterer = reg | |||
//cfg.MemberlistKV.MetricsRegisterer = reg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this was commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was removed from dskit recently
https://github.com/grafana/dskit/pull/327/files
o.presetsGauge.WithLabelValues( | ||
"max_samples_per_query", preset, | ||
).Set(float64(limits.MaxSamplesPerQuery)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was removed in cortex while removing support for chunk storage (https://github.com/cortexproject/cortex/pull/4812/files#diff-1e0a07c0c1ea87ab5a582d79c587a73a9b8e0680d89c61d5c94be106823a4a4eL61)
check limits.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Also, I forgot to mention yesterday, loki currently points to a branch of loki from main where I upgraded objstore. Please have a look. After merging the loki PR, I can point loki to |
What:
This PR upgrades many packages - starting with loki, dskit, cortex, prometheus and even golang to their latest versions.
Why:
Users of cortex-tools require support for vector expressions that were recently introduced in Loki. This change will help cortex-tools support vector expressions at the expense of changes done as described above.
Note for reviewer:
The reason for a huge PR is that most of this cannot be broken down into multiple PRs due to go mod intertwined dependencies.