runtimeconfig: initialize overridesReloadSuccess gauge in success state #2092

jgehrcke · 2020-02-07T15:14:10Z

From the commit message:

There seem to be code paths where the overridesReloadSuccess gauge
gets initialized, but `NewRuntimeConfigManager()` never is called
(absence of both log lines: "failed to load config", "runtime config
disabled: file not specified").

promauto.NewGauge() seems to initialize the gauge with the value 0
(that seems to be common in Prometheus client libs).

In the current code, the value 0 is associated with the error
state, and the value 1 is associated with the success state.

Likewise, https://github.com/grafana/cortex-jsonnet/ defines
an alert that fires when this gauge is set to 0 for an extended
period of time.

That is, when `NewRuntimeConfigManager()` never gets called
the gauge indicates a false negative error state.

Starting in the success state is a pragmatic way to solve that
problem.

It might be better to use a counter for individual error or
-- if a gauge is preferred -- one could introduce a third
state referring to "initialized".

(work in progress, let's see what CI says)

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

Tests updated -- note: all tests seem to have passed. I think I need some guidance if I am supposed to add a test.
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

jgehrcke · 2020-02-07T15:34:39Z

make test passed locally.

jgehrcke · 2020-02-07T15:35:09Z

Would appreciate some initial feedback. Thank you! :)

pstibrany · 2020-02-07T16:09:06Z

Thanks for your contribution.

RuntimeConfigManager is initialized by components that need it (ingester, querier, distributor, query-frontend, ruler, or when using "all"), but there are some components that don't use it currently (table-manager, alert-manager or compactor).

Does that match what you've observed?

pracucci

Thanks for your contribution! I accidentally run into this issue today. However, I'm not sure this is the right way to fix it. We should not export the metric at all when not used. My suggestion is to use the pattern we've recently started using:

No overridesReloadSuccess global variable, but move it to the Manager struct
Pick registerer prometheus.Registerer in input to NewRuntimeConfigManager() as second parameter and register the overridesReloadSuccess metric on that
In modules.go the initRuntimeConfig() calls NewRuntimeConfigManager(cfg.RuntimeConfig, prometheus.DefaultRegisterer)

jgehrcke · 2020-02-11T14:56:03Z

Does that match what you've observed?

Yes, I think so!

We should not export the metric at all when not used.

Agreed, that's a cleaner solution.

There seem to be code paths where the overridesReloadSuccess gauge gets initialized, but `NewRuntimeConfigManager()` never is called (absence of both log lines: "failed to load config", "runtime config disabled: file not specified"). promauto.NewGauge() seems to initialize the gauge with the value 0 (that seems to be common in Prometheus client libs). In the current code, the value 0 is associated with the error state, and the value 1 is associated with the success state. Likewise, https://github.com/grafana/cortex-jsonnet/ defines an alert that fires when this gauge is set to 0 for an extended period of time. That is, when `NewRuntimeConfigManager()` never gets called the gauge indicates a false negative error state. Starting in the success state is a pragmatic way to solve that problem. It might be better to use a counter for individual error or -- if a gauge is preferred -- one could introduce a third state referring to "initialized". Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

…ically Only register the gauge when NewRuntimeConfigManager() is called (when a runtime config manager is actually being used). Still, initialize it explicitly in state 1 (success) instead of implicitly initializing it in state (error). manager_test.go: subsequently the registry should be used for testing the state of the metric in question. Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

Adjust variable name to actual behavior: The gauge is set to 0 when loading the config from the file failed. The gauge is set to 1 when loading the config from the file succeeded. Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

jgehrcke · 2020-02-11T16:30:35Z

My suggestion is to use the pattern we've recently started using:

The commit starting with the message runtimeconfig: register cortex_overrides_last_reload_successful dynamically should do that. Thanks for the suggestions!

I have added more commits on top of that proposing variable renames in code, and after all also a rename of the gauge itself. If you can, please have a look at the commit messages. I am not sure how this affects our thinking w.r.t. backwards compatibility, but I went ahead and worded a corresponding changelog entry. If desired we can drop the last few commits again, or treat that independently in a separate discussion/PR.

pstibrany

Thank you for your work on this!

Problem with this rename is that many people (including us at Grafana Labs) have alerts based on existing name and will need to change them. That would be fair if new new name provided improved functionality, but I’m not sure it does.

Personally, I would suggest keeping previous logic and name. With your change (only registering metric when needed), if component doesn’t use runtime config manager, metric will not be exported at all. If it is, it starts with 0 (= no load of the config), and when config is successfully loaded or reloaded, it is set to 1. I think that is a good logic. Using inversion (“load failed”) is more confusing to me.

pstibrany · 2020-02-11T19:37:20Z

pkg/util/runtimeconfig/manager_test.go

@@ -204,8 +209,8 @@ func TestOverridesManager_StopClosesListenerChannels(t *testing.T) {
 			return val, nil
 		},
 	}
-
-	overridesManager, err := NewRuntimeConfigManager(overridesManagerConfig)
+	registry := prometheus.NewRegistry()


Typically in tests we just pass nil (unless testing metrics), and make sure that New function can accept nil.

thanks, done, commit pushed

Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

jgehrcke · 2020-02-11T21:20:28Z

Thanks for the quick feedback @pstibrany

Personally, I would suggest keeping previous logic and name.

Done. I have removed the more controversial commits!

Problem with this rename is that many people (including us at Grafana Labs) have alerts based on existing name and will need to change them. That would be fair if new new name provided improved functionality, but I’m not sure it does.

Well, it's totally ok and I respect the decision :). My thinking, though, was that cortex_overrides_last_reload_successful is a pretty nonexpressive name and it will only get harder to change it in the future.

About the gauge itself and its meaning:

I think in the long run we probably agree in the point of view that an error counter would be the more intuitive, simple, and complete solution. With the gauge we actually don't keep track of when exactly (and, with that, how frequently) the config load operation fails if it fails. Again, if this gauge is at 0 we actually can't even know for sure that a config load was attempted (with the current patch it's just way more likely :)).

Thanks again, looking forward to reading your feedback about the current patch.

pstibrany

Thank you!

pstibrany · 2020-02-12T12:36:21Z

pkg/util/runtimeconfig/manager.go

 	mgr := Manager{
 		cfg:  cfg,
 		quit: make(chan struct{}),
+		configLoadSuccess: prometheus.NewGauge(prometheus.GaugeOpts{
+			Name: "cortex_overrides_last_reload_successful",
+			Help: "Whether the last config reload attempt was successful (0: error, 1: successful).",


(0: error, 1: successful)

can be removed, this is common enough

done, commit pushed

pracucci · 2020-02-12T13:00:32Z

pkg/util/runtimeconfig/manager.go

 }

 // NewRuntimeConfigManager creates an instance of Manager and starts reload config loop based on config
-func NewRuntimeConfigManager(cfg ManagerConfig) (*Manager, error) {
+func NewRuntimeConfigManager(cfg ManagerConfig, metricsRegisterer prometheus.Registerer) (*Manager, error) {


metricsRegisterer > registerer

done, commit pushed

Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

pracucci

LGTM, thanks!

jtlisi

LGTM

jgehrcke force-pushed the jp/init-overridesReloadSuccess-as-success branch from d7a4ed9 to 66bd5cd Compare February 7, 2020 15:30

jgehrcke requested review from khaines, pracucci and gouthamve February 7, 2020 15:34

pracucci reviewed Feb 7, 2020

View reviewed changes

jgehrcke force-pushed the jp/init-overridesReloadSuccess-as-success branch from c1f5fcf to e72efe6 Compare February 11, 2020 16:03

jgehrcke force-pushed the jp/init-overridesReloadSuccess-as-success branch from e72efe6 to 63ebdaa Compare February 11, 2020 16:03

jgehrcke added 2 commits February 11, 2020 17:09

CHANGELOG.md: include entry for cortexproject#2092

d631199

Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

jgehrcke force-pushed the jp/init-overridesReloadSuccess-as-success branch from 63ebdaa to 314bd12 Compare February 11, 2020 16:13

jgehrcke added 2 commits February 11, 2020 17:27

jgehrcke force-pushed the jp/init-overridesReloadSuccess-as-success branch 2 times, most recently from 320ff5f to ad4721f Compare February 11, 2020 16:28

pstibrany reviewed Feb 11, 2020

View reviewed changes

runtimeconfig: simplify manager_test.go

9715c4c

Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

jgehrcke force-pushed the jp/init-overridesReloadSuccess-as-success branch from ad4721f to 9715c4c Compare February 11, 2020 21:02

CHANGELOG.md: update entry for cortexproject#2092

c77b8d5

Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

pstibrany approved these changes Feb 12, 2020

View reviewed changes

pracucci reviewed Feb 12, 2020

View reviewed changes

runtimeconfig: simplify varname and helptext

9b7b4c2

Signed-off-by: Dr. Jan-Philip Gehrcke <jp@opstrace.com>

pracucci approved these changes Feb 12, 2020

View reviewed changes

jtlisi approved these changes Feb 12, 2020

View reviewed changes

pracucci merged commit 3154cbe into cortexproject:master Feb 12, 2020

annanay25 mentioned this pull request Jun 24, 2020

Initialize RuntimeConfig module only if config is specified #2779

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

runtimeconfig: initialize overridesReloadSuccess gauge in success state #2092

runtimeconfig: initialize overridesReloadSuccess gauge in success state #2092

Uh oh!

jgehrcke commented Feb 7, 2020 •

edited

Loading

jgehrcke commented Feb 7, 2020

jgehrcke commented Feb 7, 2020

pstibrany commented Feb 7, 2020

pracucci left a comment

jgehrcke commented Feb 11, 2020

jgehrcke commented Feb 11, 2020 •

edited

Loading

pstibrany left a comment •

edited

Loading

pstibrany Feb 11, 2020

jgehrcke Feb 11, 2020

jgehrcke commented Feb 11, 2020

pstibrany left a comment

pstibrany Feb 12, 2020

jgehrcke Feb 12, 2020

pracucci Feb 12, 2020

jgehrcke Feb 12, 2020

pracucci left a comment

jtlisi left a comment

runtimeconfig: initialize overridesReloadSuccess gauge in success state #2092

runtimeconfig: initialize overridesReloadSuccess gauge in success state #2092

Uh oh!

Conversation

jgehrcke commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jgehrcke commented Feb 7, 2020

jgehrcke commented Feb 7, 2020

pstibrany commented Feb 7, 2020

pracucci left a comment

Choose a reason for hiding this comment

jgehrcke commented Feb 11, 2020

jgehrcke commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

pstibrany left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

pstibrany Feb 11, 2020

Choose a reason for hiding this comment

jgehrcke Feb 11, 2020

Choose a reason for hiding this comment

jgehrcke commented Feb 11, 2020

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany Feb 12, 2020

Choose a reason for hiding this comment

jgehrcke Feb 12, 2020

Choose a reason for hiding this comment

pracucci Feb 12, 2020

Choose a reason for hiding this comment

jgehrcke Feb 12, 2020

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

jtlisi left a comment

Choose a reason for hiding this comment

jgehrcke commented Feb 7, 2020 •

edited

Loading

jgehrcke commented Feb 11, 2020 •

edited

Loading

pstibrany left a comment •

edited

Loading