Dynamically generate available options for -target commandline arg

[alvinlin123]

What this PR does:

Add missing options to the command line help text for -target. The options are dynamically generated so we don't have to manually modify the text when we add new module in the future.

Testing:

➜  cortex git:(issue-2710-commandline-doc) go run cmd/cortex/main.go -modules
alertmanager
all
compactor
configs
data-purger
distributor
flusher
ingester
querier
query-frontend
ruler
store-gateway
table-manager
exit status 2

➜  cortex git:(issue-2710-commandline-doc) go run cmd/cortex/main.go -target api
...
level=warn ts=2020-06-23T08:25:27.092681Z caller=cortex.go:298 msg="'api' is not a public module, is this intended?" public-modules="alertmanager, all, compactor, configs, data-purger, distributor, flusher, ingester, querier, query-frontend, ruler, store-gateway, table-manager"

Which issue(s) this PR fixes:
Fixes #2710, #2596

Checklist

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

[pstibrany]

Thank you for your work on this. Unfortunately this goes against the idea why module manager was introduced in the first place: to allow projects that extend Cortex to dynamically add new modules (and set new depedendies). In this light, it’s not easily possible to enumerate all modules before they are actually added to the module manager.

Given that, one way to make this work may be to define special target like “modules” that would simply list all modules. (Or only modules that should be public in the first place, which can be defined by a new flag passed to RegisterModule function). It’s not as elegant as having it in the -help output, but should work.

[pstibrany]

/cc @annanay25

[pstibrany]

Related: #2596

[alvinlin123]

Hi @pstibrany thank you for your comment. Can you elaborate more on why "this goes against the idea why module manager was introduced in the first place: to allow projects that extend Cortex to dynamically add new modules"?

I checked the commit that added module manager and it seems like to "allow other projects to dynamically add new modules" was achieved by adding the module manager. My PR doesn't change anything regarding interaction with the module manager. My idea was, instead of using constant strings to describe a module, use a struct so that we can mark a module as public. The logic of registering and adding dependencies still remain the same. so even with this PR, other packages should still be able to dynamically add module through module manager.

I guess if there are other packages depending on the constant strings of module names then this PR would be problematic.

[pstibrany]

Hi @pstibrany thank you for your comment. Can you elaborate more on why "this goes against the idea why module manager was introduced in the first place: to allow projects that extend Cortex to dynamically add new modules"?

There is at least one (not open source) project extending Cortex by reusing the main Cortex structure, its flags and registering additional modules. I would like help for -target option to reflect such added modules. Since they are added to the module manager only after help string for this option has been constructed, I think the best way to do that is by saying “To get list of available modules, use -target=modules”, and to introduce such “modules” module that would only print list of registered user-visible modules, and then stop.

(This would also make sure that list is always up to date, and help string isn’t obsolete)

I checked the commit that added module manager and it seems like to "allow other projects to dynamically add new modules" was achieved by adding the module manager. My PR doesn't change anything regarding interaction with the module manager. My idea was, instead of using constant strings to describe a module, use a struct so that we can mark a module as public. The logic of registering and adding dependencies still remain the same. so even with this PR, other packages should still be able to dynamically add module through module manager.

I don’t think struct you have introduced is necessary, and I would like to avoid changing module type from string.
Instead, I suggest that we add flag to module manager register method to mark module as “public” (visible to user). To do that, I suggest that we change RegisterModule method to take variable number of flags of type “ModuleOption”, and implement one such option for this.

I guess if there are other packages depending on the constant strings of module names then this PR would be problematic.

Correct, it is a breaking change. We could do that if needed, but I don’t think we are there yet.

[alvinlin123]

@pstibrany I see, thanks for the explanation, I get the point now. Let me ponder a bit, and I will update this PR.

Thanks again!

[alvinlin123]

I have push new changes that display list of possible target values as well as throw error if given non-public target.

However, I didn't go with the -target module approach, it felt akward that you can use "module" in yaml config. Also, all the modules needs to depend on API, which is really unnecessary for just printing list of public modules.

So, I decided to introduce a -modules command line only flag:

➜  cortex git:(issue-2710-commandline-doc) go run cmd/cortex/main.go -modules
alertmanager
data-purger
flusher
query-frontend
all
distributor
store-gateway
configs
ingester
compactor
table-manager
ruler
querier
exit status 2

Below is an example when user pass in non-public target (I list out the possible values in the error message):

➜  cortex git:(issue-2710-commandline-doc) go run cmd/cortex/main.go -target ring
level=info ts=2020-06-20T06:42:36.235822Z caller=main.go:140 msg="Starting Cortex" version="(version=, branch=, revision=)"
level=error ts=2020-06-20T06:42:36.235893Z caller=log.go:140 msg="error running cortex" err="invalid target: ring; valid targets are: store-gateway, data-purger, compactor, flusher, query-frontend, ruler, configs, ingester, table-manager, distributor, alertmanager, all, querier"
exit status 1

[pstibrany]

Thank you, this looks good. I like -module idea, that is nicer than having extra module for that. I'd prefer to have single RegisterModule method with options, to reduce number of exported methods and structs. What do you think?

[alvinlin123]

Let's put all flags into module structure. Individual module options can modify module structure. (See my other comment about functional options).

since the module struct is not exported, I can't move the module option to the struct because doing so the cortex package will not be able to define an option setting function with argument of type *module.

[pstibrany]

Let's put all flags into module structure. Individual module options can modify module structure. (See my other comment about functional options).

since the module struct is not exported, I can't move the module option to the struct because doing so the cortex package will not be able to define an option setting function with argument of type *module.

All options passable to RegisterModule should be defined in the package with module manager. Other packages should not be able to define options.

[pstibrany]

This is looking really good, thank you for addressing my feedback. I have added few more minor comments.

[pstibrany]

Thanks for your work on this PR!

[alvinlin123]

Thanks for your work on this PR!

Thanks for your patience and mentorship :)

[pstibrany]

Please do let me know if you think it's still off; I happen think variable/method/class/whatever names are very important and I am more than welling to spend time to get the terms right to increase readability, so please don't run out of patience with me :)

At some point we need to stop bike-shedding this 😉 I'm fine with the current state. (I would perhaps use UserHiddenModule, but UserInvisibleModule is fine too).

[pracucci]

LGTM, thanks!