-
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: Revise hedging configuration block descriptions #5069
Docs: Revise hedging configuration block descriptions #5069
Conversation
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.
Looks good to me.
@@ -851,28 +851,31 @@ The `swift_storage_config` configures Swift as a general storage for different d | |||
[container_name: <string> | default = "cortex"] | |||
``` | |||
|
|||
## hedging_config | |||
## hedging |
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.
We should standardise on whether we include the _config
suffix; it's a bit inconsistent right now. For example, the few lines above this:
# Configures backend rule storage for Azure.
[azure: <azure_storage_config>]
# Configures backend rule storage for GCS.
[gcs: <gcs_storage_config>]
# Configures backend rule storage for S3.
[s3: <s3_storage_config>]
# Configures backend rule storage for Swift.
[swift: <swift_storage_config>]
I agree with your approach, but perhaps we can correct this in a subsequent PR?
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.
Definitely, @dannykopping . I was attempting to only address hedging-related configuration in this PR.
docs/sources/configuration/_index.md
Outdated
Calculate your latency to be when 99 percent of object storage requests have | ||
seen responses. |
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'm wondering if we should provide an example query here.
histogram_quantile(0.99,
sum(rate(cortex_gcs_request_duration_seconds_bucket[5m])
) by (le))
I think we should stick with referring to percentiles here as this is a commonly-used term when discussing latencies.
Calculate your latency to be when 99 percent of object storage requests have | |
seen responses. | |
Calculate your latency to be the 99th percentile of object storage response times. |
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've revised the description of the percentile. Let's push off the task of adding an example for later, as I'm unclear on the future and use of the Cortex metric.
Thank you @KMiller-Grafana |
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
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
PR #4826 was merged in without a documentation review.
This PR revises the descriptions of hedging configuration block knobs. It attempts to use the same formatting and description style as found in other blocks' descriptions within the documentation. It also changes the block name to just be "hedging" following the style implemented in PRs #5015 and #4787.