Add HTTP endpoint to show overrides (#1324) #3639

ethervoid · 2020-12-30T20:38:31Z

What this PR does:

This PR introduces a new endpoint that would retrieve the override configuration, per tenant, that is loaded as part of the runtime configuration

Which issue(s) this PR fixes:
Fixes #1324

Checklist

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

pstibrany

Thanks for your PR! It looks good, I've left some comments to check before we can merge.

pkg/api/api.go

pstibrany · 2021-01-04T09:59:47Z

pkg/api/handlers.go

Can we refactor common code for writing YAML response to util package? (see util.WriteJSONResponse)

Please note that this has now been done in #3645 too. To avoid duplicate work, it may be good idea to rebase on top of recent master, once #3645 is merged.

Great, thank you for pointing me to the new change. I'll wait until that PR is merged to make the rebase

I just merged #3645, so you can rebase master and just use that helper.

pkg/cortex/modules.go

CHANGELOG.md

ethervoid · 2021-01-07T18:37:53Z

@pstibrany I've made the requested changes but I'm waiting for the mentioned PR to be merged to do the last one referring to the refactoring of the YAML response code

pstibrany

Thank you, this looks good. One more detail that may be worth handling: if runtimeCfgManager.GetConfig() in the handler returns nil (which can happen if you define -runtime-config.file but file doesn't exist, then the output of /runtime_config is just "null". Perhaps some message like "runtime config file doesn't exist" would be more appropriate?

pstibrany · 2021-01-08T12:43:24Z

docs/api/_index.md

Please add a note saying that if Cortex is not configured with any -runtime-config.file, then this will not be available.

pstibrany · 2021-01-08T12:47:04Z

pkg/api/api.go

Nit: this should go between cortex imports, and not be standalone.

pstibrany · 2021-01-08T12:47:43Z

pkg/api/api.go

I'd mention it includes overrides too: "Current Runtime Config (incl. Overrides)"

pstibrany · 2021-01-08T12:48:39Z

pkg/cortex/modules.go

nit: extra empty line added

pstibrany · 2021-01-08T13:13:42Z

CHANGELOG.md

I would also mention that it includes overrides here.

pstibrany · 2021-01-08T13:15:23Z

Can we mention new /runtime_config endpoint in docs/configuration/arguments.md file ("Runtime Configuration file" section) too?

pracucci

Pretty cool! LGTM, modulo Peter's feedback.

In a separate PR, would be fantastic if you could also add the mode=diff support like in #3645 because it's a bit hard to read the output due to the large quantity of default valued fields. If you don't, please open an issue to leave it for someone else 🙏

CHANGELOG.md

pracucci · 2021-01-08T17:16:31Z

pkg/api/handlers.go

I just merged #3645, so you can rebase master and just use that helper.

pstibrany · 2021-01-08T17:48:40Z

In a separate PR, would be fantastic if you could also add the mode=diff support like in #3645 because it's a bit hard to read the output due to the large quantity of default valued fields. If you don't, please open an issue to leave it for someone else 🙏

This doesn’t apply to runtime config. /runtime_config only shows your changes (yaml file) nothing else.

This PR introduces a new endpoint that would retrieve the override configuration, per tenant, that is loaded as part of the runtime configuration Signed-off-by: Mario de Frutos <[email protected]>

Signed-off-by: Mario de Frutos <[email protected]>

ethervoid · 2021-01-08T19:24:41Z

I"ve been checking about the diff option and looks like we're setting the default values here to have it in the YAML unmarshal operation so I think we can add a diff option but it would be only for the overrides part

Signed-off-by: Mario de Frutos <[email protected]>

pstibrany · 2021-01-08T20:22:58Z

This doesn’t apply to runtime config. /runtime_config only shows your changes (yaml file) nothing else.

I am wrong about this. I tested this PR earlier today, but without actually setting any overrides 🤦 and just modifying other parts of runtime config.

I"ve been checking about the diff option and looks like we're setting the default values here to have it in the YAML unmarshal operation so I think we can add a diff option but it would be only for the overrides part

I think diff makes sense for entire runtime config, but is mostly interesting for overrides. However it is also somewhat tricky for overrides... when diffing, each per-tenant override should be diffed against default limits individually, and not the whole structure.

As Marco suggested, let's leave that for another PR to get this merged.

pstibrany

Thanks for patiently addressing our feedback. Let’s fix the nil case, and merge.

pstibrany · 2021-01-08T22:26:33Z

pkg/api/handlers.go

+	return func(w http.ResponseWriter, r *http.Request) {
+		runtimeConfig := runtimeCfgManager.GetConfig()
+		if runtimeConfig == nil {
+			http.Error(w, "runtime config file doesn't exist", http.StatusInternalServerError)


This isn’t error, it just means that overrides file is empty or doesn’t exist. Both are perfectly fine. Writing text message with code 200 is preferred.

pstibrany · 2021-01-08T22:27:37Z

docs/api/_index.md

+GET /runtime_config
+```
+
+Displays the runtime configuration currently applied to Cortex (in YAML format), including default values. Please be aware that the configuration will be available only if Cortex is configured with a valid `-runtime-config.file`.


-runtime-config.file option must be set for this endpoint to be available, but actual file doesn’t need to exist or be valid. But that’s just nitpicking.

Signed-off-by: Mario de Frutos <[email protected]>

ethervoid · 2021-01-09T09:53:59Z

@pstibrany I think that's all. I want to thank you for your amazing patiente guiding me and providing really valuable feedback. I've learn a lot about the the project and the way the thins are done so I can be more accurate for future PRs :-)

pstibrany · 2021-01-11T08:02:07Z

Thank you for addressing my feedback!

pull-request-size bot added the size/M label Dec 30, 2020

ethervoid mentioned this pull request Dec 30, 2020

Idea: Add HTTP endpoint to show overrides. #1324

Closed

pstibrany reviewed Jan 4, 2021

View reviewed changes

ethervoid force-pushed the issue_1324_overrides_endpoint branch 2 times, most recently from b24a71b to 6e3e7ff Compare January 7, 2021 18:32

pstibrany reviewed Jan 8, 2021

View reviewed changes

CHANGELOG.md Outdated

Copy link

Contributor

pstibrany Jan 8, 2021

Choose a reason for hiding this comment

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

I would also mention that it includes overrides here.

pracucci approved these changes Jan 8, 2021

View reviewed changes

ethervoid added 5 commits January 8, 2021 19:40

Add HTTP endpoint to show overrides (cortexproject#1324)

e00866f

This PR introduces a new endpoint that would retrieve the override configuration, per tenant, that is loaded as part of the runtime configuration Signed-off-by: Mario de Frutos <[email protected]>

Changes requested in the code review

e9c2e49

Signed-off-by: Mario de Frutos <[email protected]>

Change the route for overrides in the api documentation

c1e2a99

Signed-off-by: Mario de Frutos <[email protected]>

More code review changes

877e5f7

Signed-off-by: Mario de Frutos <[email protected]>

Fix anchor for api docs

6e9cca6

Signed-off-by: Mario de Frutos <[email protected]>

More code review changes

6a515c2

Signed-off-by: Mario de Frutos <[email protected]>

ethervoid force-pushed the issue_1324_overrides_endpoint branch from febd992 to 6a515c2 Compare January 8, 2021 19:33

ethervoid requested a review from pstibrany January 8, 2021 19:34

pstibrany approved these changes Jan 8, 2021

View reviewed changes

More code review changes

93edc5f

Signed-off-by: Mario de Frutos <[email protected]>

pstibrany merged commit 363eed8 into cortexproject:master Jan 11, 2021

Add HTTP endpoint to show overrides (#1324) #3639

Add HTTP endpoint to show overrides (#1324) #3639

Uh oh!

Conversation

ethervoid commented Dec 30, 2020

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ethervoid commented Jan 7, 2021

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany commented Jan 8, 2021

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany commented Jan 8, 2021

Uh oh!

ethervoid commented Jan 8, 2021

Uh oh!

pstibrany commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethervoid commented Jan 9, 2021

Uh oh!

pstibrany commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pstibrany commented Jan 8, 2021 •

edited

Loading