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 loki and related dependencies #277

Merged
merged 13 commits into from
Sep 6, 2023
Merged

Conversation

shantanualsi
Copy link
Contributor

@shantanualsi shantanualsi commented Sep 5, 2023

What:

This PR upgrades many packages - starting with loki, dskit, cortex, prometheus and even golang to their latest versions.

  • The cortex project removes support for chunk storage, which is why the chunk-tool and corresponding packages - cassandra, filter, storage have to be removed from cortex-tools
  • With the above deprecation, cortex also removes max_samples_per_query which has to be removed from overrides_exporter.go
  • Lint errors - specifically usage of io/ioutil (deprecated in go 1.16) and gokit/kit/.. is fixed
  • Go is upgraded to 1.21 to support all the upgraded modules

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.

@shantanualsi shantanualsi marked this pull request as ready for review September 5, 2023 13:43
@shantanualsi shantanualsi requested a review from a team as a code owner September 5, 2023 13:44
Copy link

@dannykopping dannykopping left a 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

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?

Copy link
Contributor Author

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

Comment on lines -111 to -113
o.presetsGauge.WithLabelValues(
"max_samples_per_query", preset,
).Set(float64(limits.MaxSamplesPerQuery))

Choose a reason for hiding this comment

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

Why was this removed?

Copy link
Contributor Author

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

Copy link

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@dannykopping dannykopping merged commit 0dc6d06 into main Sep 6, 2023
4 checks passed
@dannykopping dannykopping deleted the shantanu/upgrade-loki branch September 6, 2023 10:10
@shantanualsi
Copy link
Contributor Author

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 main here too.

grafana/loki#10451

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