Chore: Use runtimeconfig from dskit #4227
Conversation
3c3b53a to
5b08d87
Compare
There was a problem hiding this comment.
LGTM. You should probably rebase to drop the commit with flagext changes now that #4225 is merged.
@56quarters OK, can do that. |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
5b08d87 to
62809c6
Compare
pkg/loki/modules.go
Outdated
| @@ -149,7 +149,7 @@ func (t *Loki) initRuntimeConfig() (services.Service, error) { | |||
| validation.SetDefaultLimitsForYAMLUnmarshalling(t.Cfg.LimitsConfig) | |||
|
|
|||
| var err error | |||
| t.runtimeConfig, err = runtimeconfig.NewRuntimeConfigManager(t.Cfg.RuntimeConfig, prometheus.DefaultRegisterer) | |||
| t.runtimeConfig, err = runtimeconfig.New(t.Cfg.RuntimeConfig, prometheus.DefaultRegisterer, util_log.Logger) | |||
There was a problem hiding this comment.
Note that dskit's runtimeconfig has metrics without "cortex_" prefix. If prefix is not used, metrics rename is worth mentioning somewhere. Loki project may also prefer to use "loki_" prefix instead.
There was a problem hiding this comment.
@pstibrany oh yeah, good catch. Thanks! @slim-bean WDYT?
There was a problem hiding this comment.
+1 for having ability to choose prefix for runtime metrics!.
btw, Q: Looks like previously it was named with prefix cortex_ and now without prefix.
Won't that break some stuff in deployment_tools?
ksonnet/vendor/github.com/grafana/cortex-jsonnet/cortex-mixin/alerts/alerts.libsonnet
124: cortex_runtime_config_last_reload_successful == 0
ksonnet/lib/tempo-cloud-mixin/alerts.libsonnet
36: cortex_runtime_config_last_reload_successful{namespace=~"%s"} == 0
There was a problem hiding this comment.
@kavirajk I can add back the cortex_ prefix. Omitted it by mistake 🤦♂️ If we have to keep the cortex_ prefix, I can add it back here (in the PR).
Does the deployment_tools snippet you pasted relate also to Loki? I.e., Loki has to keep the cortex_ prefix on metrics?
There was a problem hiding this comment.
Thanks @aknuds1 :) I quickly checked in deployment tools repo and looks like Loki doesn't use this metric in any dashboards or alerts. The ones I pasted are from Cortex and Tempo.
I see two options:
- keep
cortex_prefix as default and have ability to change the prefix when needed. - No default, just accept prefix as argument.
In case 2, we should careful when Tempo or Cortex uses this change in their releases. (they should update the alerts accordingly)
Both options are fine IMO.
There was a problem hiding this comment.
I will fix the PR to add back the cortex_ prefix.
There was a problem hiding this comment.
Added back cortex_ prefix. I will leave it to Loki team to decide if they want to change the prefix to e.g. loki_ :)
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
What this PR does / why we need it:
Use the
runtimeconfigpackage from dskit.Which issue(s) this PR fixes:
Special notes for your reviewer:
Checklist