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

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
@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 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

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

@darxriggs darxriggs darxriggs left review comments

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

@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