-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
Docs: Fix documentation with correct default values and add missing options #4792
Conversation
Signed-off-by: Andre Ziviani <andrepziviani@gmail.com>
Signed-off-by: Andre Ziviani <andrepziviani@gmail.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs look good to me now.
is there something pending? can we merge this? |
@KMiller-Grafana friendly bump |
hi @slim-bean @dannykopping @owen-d, sorry to tag you directly but can you help here? This PR was already approved 16 days ago but not merged, I think theres some really important changes here. Thanks! |
friendly bump again 🙏 |
# a querier disconnects because of a crash and when the crashed querier is actually removed | ||
# from the tenant's shard. | ||
# CLI flag: -query-frontend.querier-forget-delay | ||
[querier_forget_delay: <duration> | default = 0s] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loki doesn't have support for querier shuffle sharding yet, I wonder if we should keep this configuration removed until it does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a problem leaving as is because its not only related to shuffle sharding but I can remove it no problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trevorwhitney any input here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have as much context as @slim-bean, are you saying this value isn't even used by loki yet because tenants don't have shards? If so I can see the point that adding this field to the docs because changing it won't actually do anything. At the same time it is a setting we have that will appear when someone hits the /config
endpoint or prints the config to stderr, so I don't think there's too much harm in documenting it to help explain how that value got there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes! Takes a village to keep these up to date!
Just a few suggestions. I did a pass to make sure none of the dynamic configuration we do in config_wrapper.go
as a part of the common configuration would change any of these defaults and it looks like we're good there. There are however a few defaults you put in that can be change dynamically in other sections, so I think we should leave defaults off those for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those updates! LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Great work 🎉
…ptions (#4792) * update docs with correct default values and add missing options Signed-off-by: Andre Ziviani <andrepziviani@gmail.com> * fix typo Signed-off-by: Andre Ziviani <andrepziviani@gmail.com> * fix typo Signed-off-by: Andre Ziviani <andrepziviani@gmail.com> * add another missing option Signed-off-by: Andre Ziviani <andrepziviani@gmail.com> * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * rephrase querier_forget_delay option * remove duplicated option * rephrase max_streams_per_user option * fix typo * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * fix wrong values * standardize boolean naming Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
…ptions (#4792) (#5115) * update docs with correct default values and add missing options Signed-off-by: Andre Ziviani <andrepziviani@gmail.com> * fix typo Signed-off-by: Andre Ziviani <andrepziviani@gmail.com> * fix typo Signed-off-by: Andre Ziviani <andrepziviani@gmail.com> * add another missing option Signed-off-by: Andre Ziviani <andrepziviani@gmail.com> * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * rephrase querier_forget_delay option * remove duplicated option * rephrase max_streams_per_user option * fix typo * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * Update docs/sources/configuration/_index.md Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> * fix wrong values * standardize boolean naming Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com> Co-authored-by: Andre Ziviani <7469258+AndreZiviani@users.noreply.github.com> Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
Signed-off-by: Andre Ziviani andrepziviani@gmail.com
What this PR does / why we need it:
Some of the default values were wrong in the documentation, I've updated some of them based on what I could find through the source code.
Also some options were missing.
Checklist
CHANGELOG.md
about the changes.