Implement configuration parameter categorization #840

aknuds1 · 2022-01-21T13:09:53Z

What this PR does:
Implement configuration parameter categorization, via struct field tags. The tag key "category" is used to signal category, which should be one of the following:

basic
advanced
experimental

If the key isn't supplied, basic will be assumed (i.e., it's the default).

Two new flags are introduced, printing advanced and experimental help respectively:

-help-advanced
-help-experimental

Which issue(s) this PR fixes:

Checklist

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

Help output

-help (note that -compactor.block-ranges is hidden)

TODOs

Categorize remaining parameters (maybe for another PR?)
Add tests

replay · 2022-01-25T11:41:56Z

I like the idea of categorizing the flags into basic, advanced and experimental, that should help to not overwhelm first-time users with options.

If I understand the current solution correctly then --help-advanced will only print the flags marked as "advanced" and --help-experimental will only print the flags marked as experimental. I'm wondering if it wouldn't make sense to treat this more like log levels where for example --help-advanced would give you the advanced help and also the "lower" levels, in this case basic, mixed together.

simonswine · 2022-01-25T11:45:22Z

I would tend to agree with @replay. I think even only having two help flags might be sufficient:

--help: Show basic flags.
--help-all: Show basic, advanced and experimental flags.

As a new user you won't be exposed to the variety of flags we have today, but when you are advanced you can always use --help-all and grep through all flags available.

Wdyt?

aknuds1 · 2022-01-25T12:02:14Z

@replay @simonswine I would be OK with both of your suggestions.

@replay WDYT of what @simonswine suggests? I think I tend towards sympathizing with @simonswine's thinking, i.e. that one can easily grep through all options.

replay · 2022-01-25T12:31:28Z

I agree, I think that the two options --help for basic options and --help-all for all options are probably sufficient to achieve the goal of not overwhelming basic users, and it makes it easy for everyone else to see/grep all the options.

aknuds1 · 2022-01-25T12:55:27Z

I agree, I think that the two options --help for basic options and --help-all for all options are probably sufficient to achieve the goal of not overwhelming basic users, and it makes it easy for everyone else to see/grep all the options.

@replay OK, I guess we're leaning towards changing to PR to implement the above scheme :) I'll just wait a bit to see what others say.

aknuds1 · 2022-01-25T14:01:05Z

I've implemented -help-all, removing -help-advanced and -help-experimental as suggested by @simonswine.

Also now prepending experimental flags with [experimental] as suggested by @pracucci.

Thanks for the feedback, everyone!

cmd/mimir/usage.go

pracucci

Good job!

[not necessarily for this PR but I would be glad if you could consider it] I think my main concern is that there's no testing and we could break it without noticing it. Also seeing the impact of future changes may not be trivial. I'm wondering if we could adopt an approach like the auto-generated config: store the rendered version of it in the repo and have a check like check-doc in CI which ensures every PR changing config updates it too. We could build mimir and run both mimir -help and mimir -help-all storing their output in 2 respective files. When review any PR affecting config changes (or any change to the usage) we'll see the rendered files changed too.

cmd/mimir/usage.go

aknuds1 · 2022-01-25T14:26:15Z

Good job!

[not necessarily for this PR but I would be glad if you could consider it] I think my main concern is that there's no testing and we could break it without noticing it. Also seeing the impact of future changes may not be trivial. I'm wondering if we could adopt an approach like the auto-generated config: store the rendered version of it in the repo and have a check like check-doc in CI which ensures every PR changing config updates it too. We could build mimir and run both mimir -help and mimir -help-all storing their output in 2 respective files. When review any PR affecting config changes (or any change to the usage) we'll see the rendered files changed too.

@pracucci I can add tests, I just wanted to gather feedback first.

Your suggestion regarding checking for help output regressions sounds like the Chromium testing strategy (regression testing based on reference text files) :D

aknuds1 · 2022-01-26T08:18:16Z

Working on adding tests today.

Signed-off-by: Arve Knudsen <[email protected]>

Co-authored-by: Mauro Stettler <[email protected]>

Signed-off-by: Arve Knudsen <[email protected]>

pracucci

Good job, LGTM! It looks a good starting point to me. I would be happy to merge this PR as is and then start working on classifying flags in follow up PRs.

cmd/mimir/main_test.go

Signed-off-by: Arve Knudsen <[email protected]>

cmd/mimir/main_test.go

replay

This looks good to me, good job!

I really like the new test which compares the generated help against one that has been committed to the repo, this will make it easy to visualize how a change in the code affects the generated output.

I would be happy to merge this PR as is and then start working on classifying flags in follow up PRs

I agree. But I wonder if we should try to define criteria based on which we decide which flags should be marked as advanced or experimental and commit that criteria to the docs? For example in docs/sources/contributing/design-patterns-and-conventions.md. I think without clear criteria there will almost certainly be inconsistency in the categorization, by defining the criteria early we can prevent such inconsistency.

How about:

Advanced:
A flag which is only required to tune Mimir for very specific scenarios, it should never be necessary to use an advanced flag to configure a Mimir cluster which stores less than 1M series and which uses a public object store like S3 or GCS.

Experimental:
A feature of which the Mimir team doesn't know or can reasonably assume that it has been used in any production environment for at least 3 months, and which isn't listed as Experimental in:

mimir/docs/sources/configuration/v1-guarantees.md

Line 32 in 25299bf

## Experimental features

.

KMiller-Grafana

I'm approving this, but hope you'll consider the changes I've suggested before merge.

KMiller-Grafana · 2022-01-26T20:58:15Z

cmd/mimir/main.go

@@ -95,24 +97,32 @@ func main() {
 	flag.IntVar(&blockProfileRate, "debug.block-profile-rate", 0, "Fraction of goroutine blocking events that are reported in the blocking profile. 1 to include every blocking event in the profile, 0 to disable.")
 	flag.BoolVar(&printVersion, "version", false, "Print application version and exit.")
 	flag.BoolVar(&printModules, "modules", false, "List available values that can be used as target.")
+	flag.BoolVar(&printHelp, "help", false, "Print basic help.")
+	flag.BoolVar(&printHelp, "h", false, "Print basic help.")
+	flag.BoolVar(&printHelpAll, "help-all", false, "Print help including also advanced and experimental flags.")


Not sure what to suggest. Configuration options are not the same as flags. Are they parameters? Knobs? Parameters is used elsewhere in user help output, so maybe do that here as well.

Suggested change

flag.BoolVar(&printHelpAll, "help-all", false, "Print help including also advanced and experimental flags.")

flag.BoolVar(&printHelpAll, "help-all", false, "Print help, also including advanced and experimental flags.")

@KMiller-Grafana I use the term flags consequently as regards the CLI, since that's what they are in this context. They correspond to configuration parameters, but are concretely flags in this user interface.

For example, -help, -h and -help-all are all flags.

I haven't double checked if we're consequent in the CLI terminology elsewhere though, that's the most important thing.

KMiller-Grafana · 2022-01-26T21:01:03Z

cmd/mimir/main.go

-		flag.CommandLine.SetOutput(os.Stdout)
-		usage()
+	if err := flag.CommandLine.Parse(os.Args[1:]); err != nil {
+		fmt.Fprintln(flag.CommandLine.Output(), "Run with -help to get list of available parameters")


Can you make this same substitution in the other lines that read the same?

Suggested change

fmt.Fprintln(flag.CommandLine.Output(), "Run with -help to get list of available parameters")

fmt.Fprintln(flag.CommandLine.Output(), "Run with -help to get a list of available parameters.")

@KMiller-Grafana this is actually carried over from old code. If we're to change it, I think it's best considered in its own PR.

@KMiller-Grafana after reading this more closely, I think the current wording ("Run with -help to get list of available parameters") is the right one, since it's not just a list, but a certain list (the list of basic parameters). Changing it would make it more ambiguous to me at least, since it makes it sound as if there's some doubt as to what list would be produced.

I made my suggestion to make the phrase grammatically correct.

@aknuds1 My opinion: shoveling this off into another PR just created technical debt that no one will ever get to or implement. I'm currently your expert when it comes to documentation of anything related to Mimir configuration. I'm also your UI person for this effort. Please, since you've argued against what I know is an improvement, make the changes in another PR yourself, and don't lose the task.

@KMiller-Grafana I already made another PR, which has been merged in the meantime.

pracucci · 2022-01-27T07:27:03Z

But I wonder if we should try to define criteria based on which we decide which flags should be marked as advanced or experimental and commit that criteria to the docs?

💯 agree. I know @09jvilla and team already did some work (I reviewed a classification proposal just yesterday) but I also missed a bit the rationale behind some classification. Given we're going public, I definitely agree on documenting the rationale in this doc so that we have a guideline to enforce it over time.

Going to merge this PR since the classification criteria doesn't block the foundation work done here.

pracucci · 2022-01-27T07:27:55Z

I'm approving this, but hope you'll consider the changes I've suggested before merge.

Sorry! I totally missed the comment before merging. @aknuds1 can you address it in a follow up PR, please?

aknuds1 · 2022-01-27T07:31:29Z

@pracucci will do.

aknuds1 force-pushed the feat/categorize-flags branch from 258a5e8 to 697d1a8 Compare January 21, 2022 13:20

osg-grafana requested review from osg-grafana and KMiller-Grafana January 21, 2022 14:16

osg-grafana added the type/docs Improvements or additions to documentation label Jan 21, 2022

aknuds1 added the enhancement New feature or request label Jan 24, 2022

aknuds1 force-pushed the feat/categorize-flags branch from 697d1a8 to aa38e00 Compare January 25, 2022 07:33

aknuds1 changed the title ~~WIP: Implement configuration parameter categorization~~ Implement configuration parameter categorization Jan 25, 2022

aknuds1 marked this pull request as ready for review January 25, 2022 08:57

aknuds1 requested a review from a team January 25, 2022 08:58

aknuds1 removed the type/docs Improvements or additions to documentation label Jan 25, 2022

aknuds1 force-pushed the feat/categorize-flags branch from f301f0a to dd341e6 Compare January 25, 2022 13:51

replay reviewed Jan 25, 2022

View reviewed changes

cmd/mimir/usage.go Outdated Show resolved Hide resolved

replay reviewed Jan 25, 2022

View reviewed changes

cmd/mimir/usage.go Outdated Show resolved Hide resolved

pracucci approved these changes Jan 25, 2022

View reviewed changes

cmd/mimir/usage.go Outdated Show resolved Hide resolved

cmd/mimir/usage.go Outdated Show resolved Hide resolved

aknuds1 force-pushed the feat/categorize-flags branch from 7157391 to 0bbd263 Compare January 25, 2022 15:10

aknuds1 force-pushed the feat/categorize-flags branch 5 times, most recently from aff1318 to e87c41c Compare January 26, 2022 15:44

aknuds1 requested a review from replay January 26, 2022 15:44

cmd/mimir: Detect flags' categories

957e4e4

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 and others added 15 commits January 26, 2022 16:57

Move experimental prefix before usage string instead

ed8821f

Signed-off-by: Arve Knudsen <[email protected]>

cmd/mimir: Detect flags' categories

9c9d69f

Signed-off-by: Arve Knudsen <[email protected]>

Implement help flag explicitly

6a01edf

Signed-off-by: Arve Knudsen <[email protected]>

Add advanced and experimental help flags

ff5507e

Signed-off-by: Arve Knudsen <[email protected]>

Fix printing of non-configuration param flags

e108131

Signed-off-by: Arve Knudsen <[email protected]>

Fix quoting of default string values

5be49db

Signed-off-by: Arve Knudsen <[email protected]>

Add to changelog

91e2fd8

Signed-off-by: Arve Knudsen <[email protected]>

Introduce -help-all flag instead of -help-advanced etc

36c09d4

Signed-off-by: Arve Knudsen <[email protected]>

Print [experimental] before experimental flags

57adf25

Signed-off-by: Arve Knudsen <[email protected]>

Update cmd/mimir/usage.go

ea2a430

Co-authored-by: Mauro Stettler <[email protected]>

Move experimental prefix before usage string instead

f417078

Signed-off-by: Arve Knudsen <[email protected]>

Remove special formatting of one-character flags

cb095e1

Signed-off-by: Arve Knudsen <[email protected]>

Document that the basic flag category is the default

279cadb

Signed-off-by: Arve Knudsen <[email protected]>

Add tests

8be548d

Signed-off-by: Arve Knudsen <[email protected]>

Add Make target reference-help

83b4e96

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 force-pushed the feat/categorize-flags branch from e87c41c to 83b4e96 Compare January 26, 2022 15:57

pracucci approved these changes Jan 26, 2022

View reviewed changes

cmd/mimir/main_test.go Show resolved Hide resolved

Add hint to -help output

27633e8

Signed-off-by: Arve Knudsen <[email protected]>

aknuds1 force-pushed the feat/categorize-flags branch from 47cfa05 to 27633e8 Compare January 26, 2022 17:07

replay reviewed Jan 26, 2022

View reviewed changes

cmd/mimir/main_test.go Show resolved Hide resolved

replay approved these changes Jan 26, 2022

View reviewed changes

KMiller-Grafana approved these changes Jan 26, 2022

View reviewed changes

pracucci merged commit f2ca89c into main Jan 27, 2022

pracucci deleted the feat/categorize-flags branch January 27, 2022 07:27

osg-grafana added the type/docs Improvements or additions to documentation label Jan 27, 2022

aknuds1 mentioned this pull request Jan 27, 2022

Tag advanced and experimental configuration parameter fields #912

Closed

	flag.BoolVar(&printHelpAll, "help-all", false, "Print help including also advanced and experimental flags.")
	flag.BoolVar(&printHelpAll, "help-all", false, "Print help, also including advanced and experimental flags.")

	fmt.Fprintln(flag.CommandLine.Output(), "Run with -help to get list of available parameters")
	fmt.Fprintln(flag.CommandLine.Output(), "Run with -help to get a list of available parameters.")

Implement configuration parameter categorization #840

Implement configuration parameter categorization #840

Uh oh!

Conversation

aknuds1 commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Help output

TODOs

Uh oh!

replay commented Jan 25, 2022

Uh oh!

simonswine commented Jan 25, 2022

Uh oh!

aknuds1 commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

replay commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aknuds1 commented Jan 25, 2022

Uh oh!

aknuds1 commented Jan 25, 2022

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aknuds1 commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aknuds1 commented Jan 26, 2022

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

replay 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

Uh oh!

KMiller-Grafana left a comment

Choose a reason for hiding this comment

Uh oh!

KMiller-Grafana Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KMiller-Grafana Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KMiller-Grafana Jan 27, 2022

Choose a reason for hiding this comment

Uh oh!

aknuds1 Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci commented Jan 27, 2022

Uh oh!

pracucci commented Jan 27, 2022

Uh oh!

aknuds1 commented Jan 27, 2022

Uh oh!

Uh oh!

aknuds1 commented Jan 21, 2022 •

edited

Loading

aknuds1 commented Jan 25, 2022 •

edited

Loading

replay commented Jan 25, 2022 •

edited

Loading

aknuds1 commented Jan 25, 2022 •

edited

Loading

replay left a comment •

edited

Loading

aknuds1 Jan 27, 2022 •

edited

Loading

aknuds1 Jan 27, 2022 •

edited

Loading

aknuds1 Jan 27, 2022 •

edited

Loading