Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Document the common config section #4485

Merged
merged 2 commits into from
Oct 25, 2021
Merged

Document the common config section #4485

merged 2 commits into from
Oct 25, 2021

Conversation

DylanGuedes
Copy link
Contributor

@DylanGuedes DylanGuedes commented Oct 15, 2021 •
edited
Loading

What this PR does / why we need it:

  • Documents the common Loki config section.
  • Adds a bunch of missing periods (from same config section)

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

Special notes for your reviewer:
PR #4473 will introduce the object_storage subsection to common. path_prefix already exists (although it isn't documented yet)

Checklist

  • Documentation added
  • Tests updated

@DylanGuedes DylanGuedes mentioned this pull request Oct 18, 2021
Merged
2 tasks
@DylanGuedes DylanGuedes force-pushed the document-storage-common-config branch from ca22b22 to bf5bb20 Compare October 20, 2021 15:38
@pull-request-size pull-request-size bot added size/L and removed size/S labels Oct 20, 2021
@DylanGuedes DylanGuedes force-pushed the document-storage-common-config branch 2 times, most recently from 2cbda26 to 14d7913 Compare October 25, 2021 12:56
@pull-request-size pull-request-size bot added size/M and removed size/L labels Oct 25, 2021
@DylanGuedes DylanGuedes force-pushed the document-storage-common-config branch from 14d7913 to 2123679 Compare October 25, 2021 12:58
@DylanGuedes DylanGuedes force-pushed the document-storage-common-config branch from 2123679 to d80b0c4 Compare October 25, 2021 13:22
@DylanGuedes DylanGuedes changed the title WIP: Document the common config section Document the common config section Oct 25, 2021
@DylanGuedes DylanGuedes marked this pull request as ready for review October 25, 2021 13:24
cyriltovena
cyriltovena approved these changes Oct 25, 2021
View reviewed changes
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena
Copy link
Contributor

@slim-bean can you double check ? seems good to me.

@DylanGuedes
Copy link
Contributor Author

(adding Trevor as a reviewer since he wrote the common storage config)

owen-d
owen-d reviewed Oct 25, 2021
View reviewed changes
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

corrected a few doc references

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
@DylanGuedes @owen-d
Apply suggestions from code review
96ae635 
Fix subsection reference from `filesystem_storage_config` to `local_storage_config`

Co-authored-by: Owen Diehl <ow.diehl@gmail.com>
@owen-d owen-d merged commit e0b4e25 into grafana:main Oct 25, 2021
@pgassmann
Copy link
Contributor

the documentation for the common_config section is not clear to me.

path_prefix: the description sounds like it is an url prefix, but from the examples it looks like it is the filesystem path.

storage/filesystem/chunks_directory and rules_directory is present in the 2.4 release notes examples, but not documented.

@DylanGuedes
Copy link
Contributor Author

the documentation for the common_config section is not clear to me.

path_prefix: the description sounds like it is an url prefix, but from the examples it looks like it is the filesystem path.

My bad, you're totally right - the current description is misleading, the path_prefix as you said is actually used to mount filesystem paths to read files from.

storage/filesystem/chunks_directory and rules_directory is present in the 2.4 release notes examples, but not documented.

Actually, these two options were added after this PR, so it makes sense to not cover them at this same PR.

I'll create an issue regarding these two problems that you mentioned. Thank you for reporting them!

darxriggs
darxriggs reviewed Mar 30, 2022
View reviewed changes
docs/sources/configuration/_index.md
# A common storage config to be used by the different Loki components.
[storage: <common_storage_config>]

# When defined, the given prefix will be present in front of the endpoint paths.
Copy link
Contributor

@darxriggs darxriggs Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This sentence is misleading. This path_prefix can be about internal directories for local storage and about URLs for remote storage - right?

See config_wrapper.go here and examples like loki-local-config.yaml here and simple-scalable/loki-config.yaml here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is wrong, I created this issue after this PR got merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owen-d owen-d owen-d approved these changes

@darxriggs darxriggs darxriggs left review comments

@cyriltovena cyriltovena cyriltovena approved these changes

@KMiller-Grafana KMiller-Grafana Awaiting requested review from KMiller-Grafana

@trevorwhitney trevorwhitney Awaiting requested review from trevorwhitney

@dannykopping dannykopping Awaiting requested review from dannykopping

@slim-bean slim-bean Awaiting requested review from slim-bean

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document the Common Config
5 participants
@DylanGuedes @cyriltovena @pgassmann @darxriggs @owen-d