Add configuration documentation generation tool #7916

ssncferreira · 2022-12-12T19:27:42Z

What this PR does / why we need it:

Add a tool to generate configuration flags documentation based on the flags properties defined on registration on the code. This tool is based on the Mimir doc generation tool and adapted according to Loki configuration specifications.

Prior to this PR, the configuration flags documentation was dispersed across two sources:

_index.md
configuration flags registration in the code

This meant that there was no single source of truth.
In this PR, the previous _index.md file is replaced with the new file generated by the tool.

The next step includes adding a CI step that validates if the _index.md file was generated according to the flags settings. This will be done in a follow-up PR.

NOTE: this is not a documentation update PR. Apart from some minor typo fixes, the documentation changes on the code, were copied from the _index.md file.

Which issue(s) this PR fixes:
Fixes https://github.com/grafana/loki-private/issues/83

Special notes for your reviewer:

Files:

docs/sources/configuration/index.template: template used to generate the final configuration file
/docs/sources/configuration/_index.md: file generated by tool
loki/pkg directory files updated with up-to-date documentation from _index.md file
tools/doc-generator directory with documentation generation tool.

Checklist

Reviewed the CONTRIBUTING.md guide
Documentation added
Tests updated
CHANGELOG.md updated
Changes that require user attention or interaction to upgrade are documented in docs/sources/upgrading/_index.md

periklis

Couple of things that I noticed. Great piece of work! Thank you so much 🚀

periklis · 2022-12-12T19:49:43Z

pkg/loki/common/common.go

+	s.S3.RegisterFlagsWithPrefix(prefix, f)
+	s.GCS.RegisterFlagsWithPrefix(prefix, f)
+	s.Azure.RegisterFlagsWithPrefix(prefix, f)
+	s.Swift.RegisterFlagsWithPrefix(prefix, f)
+	s.BOS.RegisterFlagsWithPrefix(prefix, f)


This looks like a breaking change that needs extra mention in the backlog, right?

Yes, this is a breaking change to the CLI flags, because these were registered incorrectly. Not sure what the best course of action is here 😕 See comment: #7916 (comment)

I think for the sake of BC, we should keep the old flags and put a deprecation notice in the upgrade guide, and create an issue for v3.0 to remove these deprecated flags.

As discussed internally via slack, this is not a breaking change because the CLI flags with the prefix common.storage are not yet available to the default flag system. These are only set on the throwaway flag system to allow for common config application logic.

However, in order to be able to generate the documentation for these flags, they need to be set on the default flag system: https://github.com/grafana/loki/pull/7916/files#diff-5789ed7c47e1cf4394cef6ad3bc4aaea1af3552e930b0420703708e6bd7d2c89R54-R57

periklis · 2022-12-12T19:50:48Z

pkg/loki/loki.go

+	CompactorHTTPClient compactor_client.HTTPConfig `yaml:"compactor_client,omitempty" doc:"hidden"`
+	CompactorGRPCClient compactor_client.GRPCConfig `yaml:"compactor_grpc_client,omitempty" doc:"hidden"`


Why are these clients hidden? At least the GRPC client should be visible for users to configure TLS settings.

These flags were added recently and were still not yet documented in the _index.md file. Therefore, added the hidden flag for now. Nevertheless, will remove it in a follow-up PR in order to generate the documentation associated 👍

ssncferreira · 2022-12-13T11:25:58Z

There are a couple of breaking changes on the CLI flags:

ruler.wal-cleaer.period typo in cleaner.
Storage types are registered with 2 duplicate prefixes. Example for gcs: common.storage.gcsgcs.bucketname. This PR changes this flag to common.storage.gcs.bucketname.
Similarly to storage type bos, it was registered as common.storage.bosbaidubce. This PR changes the prefix to common.storage.bos for consistency. Edit: this is not a breaking change because the CLI flags with the prefix common.storage are not yet available to the default flag system. These are only set on the throwaway flag system to allow for common config application logic. (see comment Add configuration documentation generation tool #7916 (comment))

What is the best course of action here? Should we keep the previous flags, deprecate them and create new ones?

ssncferreira · 2022-12-13T11:29:29Z

pkg/ruler/storage/cleaner/config.go

@@ -14,7 +15,7 @@ type Config struct {
 	Period time.Duration `yaml:"period,omitempty"`
 }

-func (c Config) RegisterFlags(f *flag.FlagSet) {
+func (c *Config) RegisterFlags(f *flag.FlagSet) {


This pointer in config was missing, meaning that the default values for the variables were not taken into account. This might affect deployments using the ruler WAL cleaner config without any values defined.

I think we should add an item to the upgrade guide about this

Upgrade guide makes sense 👍 Will address this in a follow-up PR to avoid increasing the size and review of this one

dannykopping

Amazing work @ssncferreira! Thanks for taking on this very detail-oriented and labour-intensive task.

This PR is obviously too big to review closely, but generally everything LGTM. I think we need to add some upgrade notes but besides that I'm happy. So glad we have this now!!

dannykopping · 2022-12-13T11:33:29Z

pkg/logql/engine.go

@@ -113,7 +113,7 @@ type Querier interface {
 type EngineOpts struct {
 	// TODO: remove this after next release.
 	// Timeout for queries execution
-	Timeout time.Duration `yaml:"timeout"`
+	Timeout time.Duration `yaml:"timeout" doc:"deprecated"`


Possible enhancement for later: add a deprecated flag so we can easily identify these

dannykopping · 2022-12-13T11:34:36Z

pkg/loki/common/common.go

+	s.S3.RegisterFlagsWithPrefix(prefix, f)
+	s.GCS.RegisterFlagsWithPrefix(prefix, f)
+	s.Azure.RegisterFlagsWithPrefix(prefix, f)
+	s.Swift.RegisterFlagsWithPrefix(prefix, f)
+	s.BOS.RegisterFlagsWithPrefix(prefix, f)


I think for the sake of BC, we should keep the old flags and put a deprecation notice in the upgrade guide, and create an issue for v3.0 to remove these deprecated flags.

dannykopping · 2022-12-13T11:35:18Z

pkg/ruler/storage/cleaner/config.go

@@ -14,7 +15,7 @@ type Config struct {
 	Period time.Duration `yaml:"period,omitempty"`
 }

-func (c Config) RegisterFlags(f *flag.FlagSet) {
+func (c *Config) RegisterFlags(f *flag.FlagSet) {


I think we should add an item to the upgrade guide about this

grafanabot · 2022-12-13T15:03:10Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-12-14T10:20:34Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

grafanabot · 2022-12-14T10:33:34Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

dannykopping

LGTM!

grafanabot · 2022-12-14T10:39:44Z

./tools/diff_coverage.sh ../loki-target-branch/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0.1%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
+               loki	0%

chaudum

Great job! A few nits, but these should not block you.

chaudum · 2022-12-14T10:30:37Z

docs/sources/configuration/index.template

+
+Configuration examples can be found in the [Configuration Examples](examples/) document.
+
+## Printing Loki Config At Runtime


I think we use sentence case for titles

Suggested change

## Printing Loki Config At Runtime

## Printing Loki config at runtime

See also other headlines in this document

👍 sounds good. Will address these comments in a separate PR

chaudum · 2022-12-14T10:34:33Z

pkg/ingester/wal.go

@@ -40,14 +40,14 @@ func (cfg *WALConfig) Validate() error {

 // RegisterFlags adds the flags required to config this to the given FlagSet
 func (cfg *WALConfig) RegisterFlags(f *flag.FlagSet) {
-	f.StringVar(&cfg.Dir, "ingester.wal-dir", "wal", "Directory to store the WAL and/or recover from WAL.")
+	f.StringVar(&cfg.Dir, "ingester.wal-dir", "wal", "Directory where the WAL data should be stored and/or recovered from.")


It's not a suggestion (should) but a fact (is).

Suggested change

f.StringVar(&cfg.Dir, "ingester.wal-dir", "wal", "Directory where the WAL data should be stored and/or recovered from.")

f.StringVar(&cfg.Dir, "ingester.wal-dir", "wal", "Directory where the WAL data is stored and/or recovered from.")

chaudum · 2022-12-14T10:36:27Z

pkg/querier/querier.go

 }

 // RegisterFlags register flags.
 func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	cfg.Engine.RegisterFlagsWithPrefix("querier", f)
-	f.DurationVar(&cfg.TailMaxDuration, "querier.tail-max-duration", 1*time.Hour, "Limit the duration for which live tailing request would be served")
+	f.DurationVar(&cfg.TailMaxDuration, "querier.tail-max-duration", 1*time.Hour, "Maximum duration for which the live tailing requests should be served.")


Suggested change

f.DurationVar(&cfg.TailMaxDuration, "querier.tail-max-duration", 1*time.Hour, "Maximum duration for which the live tailing requests should be served.")

f.DurationVar(&cfg.TailMaxDuration, "querier.tail-max-duration", 1*time.Hour, "Maximum duration for which the live tailing requests are served.")

chaudum · 2022-12-14T10:37:03Z

pkg/ruler/base/ruler.go

-	f.StringVar(&cfg.RulePath, "ruler.rule-path", "/rules", "file path to store temporary rule files for the prometheus rule managers")
-	f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler api")
+	f.StringVar(&cfg.RulePath, "ruler.rule-path", "/rules", "File path to store temporary rule files.")
+	f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler api.")


Suggested change

f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler api.")

f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler API.")

chaudum · 2022-12-14T10:37:34Z

pkg/ruler/config.go

@@ -31,10 +31,10 @@ func (c *Config) RegisterFlags(f *flag.FlagSet) {
 	c.WALCleaner.RegisterFlags(f)

 	// TODO(owen-d, 3.0.0): remove deprecated experimental prefix in Cortex if they'll accept it.
-	f.BoolVar(&c.Config.EnableAPI, "ruler.enable-api", true, "Enable the ruler api")
+	f.BoolVar(&c.Config.EnableAPI, "ruler.enable-api", true, "Enable the ruler api.")


Suggested change

f.BoolVar(&c.Config.EnableAPI, "ruler.enable-api", true, "Enable the ruler api.")

f.BoolVar(&c.Config.EnableAPI, "ruler.enable-api", true, "Enable the ruler API.")

**What this PR does / why we need it**: Fix documentation typos from #7916 (review) **Which issue(s) this PR fixes**: Fixes #7916 (review) **Special notes for your reviewer**: **Checklist** - [ ] Reviewed the `CONTRIBUTING.md` guide - [ ] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`

…er.wal-cleaner.period (#7937) **What this PR does / why we need it**: Deprecate CLI flag `-ruler.wal-cleaer.period` in favor of CLI flag `-ruler.wal-cleaner.period`. **Which issue(s) this PR fixes**: Fixes #7916 (comment) and #7916 (comment) **NOTE:** correctly registring the `ruler.wal-cleaner` configuration flags is not a breaking change and doesn't require an upgrade guide, because the ruler WAL cleaner initialization code already guarantees that the default values are used: https://github.com/grafana/loki/blob/c71620ae9437aa23142cdc410115f28ae8f29997/pkg/ruler/storage/cleaner/cleaner.go#L76-L101 **Special notes for your reviewer**: **Checklist** - [x] Reviewed the `CONTRIBUTING.md` guide - [x] Documentation added - [ ] Tests updated - [x] `CHANGELOG.md` updated - [x] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`

**What this PR does / why we need it**: Follow up from PR: #7916 to add a CI step to check if the configuration flag documentation file `doc/sources/configuration/_index.md` was generated successfully. Requirements: * CI `check` pipeline should fail if the documentation file is updated manually * CI `check` pipeline should fail if registration flags are changed in the code, but the documentation file is not generated. **Which issue(s) this PR fixes**: Fixes https://github.com/grafana/loki-private/issues/83 **Special notes for your reviewer**: **Checklist** - [ ] Reviewed the `CONTRIBUTING.md` guide - [ ] Documentation added - [ ] Tests updated - [x] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`

**What this PR does / why we need it**: PR #7916 which introduced the documentation automation tool for Loki's configuration values, incorrectly removed the last sections of the documentation. This PR introduces them accordingly. **Checklist** - [ ] Reviewed the [`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md) guide (**required**) - [x] Documentation added - [ ] Tests updated - [ ] `CHANGELOG.md` updated - [ ] Changes that require user attention or interaction to upgrade are documented in `docs/sources/upgrading/_index.md`

With the addition of the [config generation tool](#7916), a comment prefix was added for all cache types: `Cache config for index entry writing.` https://grafana.com/docs/loki/next/configuration/#cache_config This appears in the description of each cache config option, and is misleading.

With the addition of the [config generation tool](#7916), a comment prefix was added for all cache types: `Cache config for index entry writing.` https://grafana.com/docs/loki/next/configuration/#cache_config This appears in the description of each cache config option, and is misleading. (cherry picked from commit 2d93952)

pull-request-size bot added the size/XXL label Dec 12, 2022

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Dec 12, 2022

ssncferreira force-pushed the doc_gen_tool branch 2 times, most recently from 56fa19d to a1c1e83 Compare December 12, 2022 19:44

periklis reviewed Dec 12, 2022

View reviewed changes

ssncferreira force-pushed the doc_gen_tool branch 2 times, most recently from 160a0a0 to 5550cd6 Compare December 13, 2022 10:43

Add configuration documentation generation tool

754d6f8

ssncferreira force-pushed the doc_gen_tool branch from 5550cd6 to 754d6f8 Compare December 13, 2022 11:03

ssncferreira commented Dec 13, 2022

View reviewed changes

dannykopping approved these changes Dec 13, 2022

View reviewed changes

Fix lint

9de067b

Keep CLI flags with typos

cc52fa4

ssncferreira force-pushed the doc_gen_tool branch from 6c031c7 to cc52fa4 Compare December 13, 2022 16:43

Add common.storage CLI flags

94677a5

ssncferreira marked this pull request as ready for review December 14, 2022 10:14

ssncferreira requested a review from a team as a code owner December 14, 2022 10:14

Merge branch 'main' into doc_gen_tool

3e3461a

Replace _index.md with index generated by doc generation tool

c32e5d0

dannykopping approved these changes Dec 14, 2022

View reviewed changes

chaudum reviewed Dec 14, 2022

View reviewed changes

ssncferreira merged commit f93b91b into main Dec 14, 2022

ssncferreira deleted the doc_gen_tool branch December 14, 2022 10:47

ssncferreira added a commit that referenced this pull request Dec 14, 2022

Fix documentation typos from #7916

1622629

ssncferreira mentioned this pull request Dec 14, 2022

Fix documentation typos from #7916 #7934

Merged

5 tasks

ssncferreira mentioned this pull request Dec 14, 2022

Deprecate CLI flag -ruler.wal-cleaer.period in favor of CLI flag -ruler.wal-cleaner.period #7937

Merged

5 tasks

ssncferreira mentioned this pull request Dec 14, 2022

Add CI step to check generated documentation to check pipeline #7938

Merged

5 tasks

ssncferreira mentioned this pull request Jan 19, 2023

Docs fix: add missing sections on index.template file #8216

Merged

5 tasks

dannykopping mentioned this pull request Mar 21, 2023

(chore): remove incorrect cache config preample #8859

Merged

JStickler mentioned this pull request Mar 11, 2024

Adding JSON schema for configuration file #12083

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration documentation generation tool #7916

Add configuration documentation generation tool #7916

ssncferreira commented Dec 12, 2022 •

edited

Loading

periklis left a comment

periklis Dec 12, 2022

ssncferreira Dec 13, 2022

dannykopping Dec 13, 2022

ssncferreira Dec 13, 2022 •

edited

Loading

periklis Dec 12, 2022

ssncferreira Dec 14, 2022

ssncferreira commented Dec 13, 2022 •

edited

Loading

ssncferreira Dec 13, 2022

dannykopping Dec 13, 2022

ssncferreira Dec 14, 2022

dannykopping left a comment

dannykopping Dec 13, 2022

dannykopping Dec 13, 2022

dannykopping Dec 13, 2022

grafanabot commented Dec 13, 2022

grafanabot commented Dec 14, 2022

grafanabot commented Dec 14, 2022

dannykopping left a comment

grafanabot commented Dec 14, 2022

chaudum left a comment

chaudum Dec 14, 2022

chaudum Dec 14, 2022

ssncferreira Dec 14, 2022

chaudum Dec 14, 2022

chaudum Dec 14, 2022

chaudum Dec 14, 2022

chaudum Dec 14, 2022

		CompactorHTTPClient compactor_client.HTTPConfig `yaml:"compactor_client,omitempty" doc:"hidden"`
		CompactorGRPCClient compactor_client.GRPCConfig `yaml:"compactor_grpc_client,omitempty" doc:"hidden"`


		Configuration examples can be found in the [Configuration Examples](examples/) document.

		## Printing Loki Config At Runtime

	f.StringVar(&cfg.Dir, "ingester.wal-dir", "wal", "Directory where the WAL data should be stored and/or recovered from.")
	f.StringVar(&cfg.Dir, "ingester.wal-dir", "wal", "Directory where the WAL data is stored and/or recovered from.")

	f.DurationVar(&cfg.TailMaxDuration, "querier.tail-max-duration", 1*time.Hour, "Maximum duration for which the live tailing requests should be served.")
	f.DurationVar(&cfg.TailMaxDuration, "querier.tail-max-duration", 1*time.Hour, "Maximum duration for which the live tailing requests are served.")

	f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler api.")
	f.BoolVar(&cfg.EnableAPI, "experimental.ruler.enable-api", false, "Enable the ruler API.")

	f.BoolVar(&c.Config.EnableAPI, "ruler.enable-api", true, "Enable the ruler api.")
	f.BoolVar(&c.Config.EnableAPI, "ruler.enable-api", true, "Enable the ruler API.")

Add configuration documentation generation tool #7916

Add configuration documentation generation tool #7916

Conversation

ssncferreira commented Dec 12, 2022 • edited Loading

periklis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssncferreira Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssncferreira commented Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannykopping left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grafanabot commented Dec 13, 2022

grafanabot commented Dec 14, 2022

grafanabot commented Dec 14, 2022

dannykopping left a comment

Choose a reason for hiding this comment

grafanabot commented Dec 14, 2022

chaudum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssncferreira commented Dec 12, 2022 •

edited

Loading

ssncferreira Dec 13, 2022 •

edited

Loading

ssncferreira commented Dec 13, 2022 •

edited

Loading