Introduce http config settings in Azure storage
#4581
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cortex v1.11.0 included thanos-io/thanos#3970, which added configuration options to Azure's http client and transport, replacing usage of `http.DefaultClient`. Unfortunately since Cortex was not setting this config, Cortex implicitly switched from `http.DefaultClient` to all empty values (e.g. `MaxIdleConns: 0` rather than 100). Introduce `http` config settings to Azure storage. This motivated moving `s3.HTTPConfig` into a new `pkg/storage/bucket/config` package, to allow `azure` and `s3` to share it. Also update the instructions for running the website to include installing `embedmd`. Signed-off-by: Andrew Seigner <andrew@sig.gy>
bboreham
approved these changes
Dec 9, 2021
CHANGELOG.md
Outdated
| @@ -3,14 +3,15 @@ | |||
| ## master / unreleased | |||
|
|
|||
| * [CHANGE] Changed default for `-ingester.min-ready-duration` from 1 minute to 15 seconds. #4539 | |||
| * [ENHANCEMENT] Ruler: Add `ruler.disable-rule-group-label` to disable the `rule_group` label on exported metrics. #4571 | |||
| * [CHANGE] query-frontend: Do not print anything in the logs of `query-frontend` if a in-progress query has been canceled (context canceled). #4562 | |||
There was a problem hiding this comment.
Unrelated change - please move to a different PR.
There was a problem hiding this comment.
Posted #4584, will remove from here.
pkg/storage/bucket/config/http.go
Outdated
| @@ -0,0 +1,35 @@ | |||
| package config | |||
There was a problem hiding this comment.
Small thing, but I got a little confused over the naming of this package.
It doesn't contain all bucket config. I think it exists to avoid a circular dependency?
Given what it does, I might call the package http and the struct Config.
There was a problem hiding this comment.
Yup it was to avoid the circular dependency. Alternatively we could make azure depend on s3.HTTPConfig, but that seemed less clear, also s3.HTTPConfig has a Transport field that would be unused by azure.
I've renamed config.HTTP -> http.Config.
also back out changelog cleanup Signed-off-by: Andrew Seigner <andrew@sig.gy>
Signed-off-by: Andrew Seigner <andrew@sig.gy>
bboreham
approved these changes
Dec 10, 2021
alvinlin123
pushed a commit
to ac1214/cortex
that referenced
this pull request
Jan 14, 2022
* Introduce `http` config settings in Azure storage Cortex v1.11.0 included thanos-io/thanos#3970, which added configuration options to Azure's http client and transport, replacing usage of `http.DefaultClient`. Unfortunately since Cortex was not setting this config, Cortex implicitly switched from `http.DefaultClient` to all empty values (e.g. `MaxIdleConns: 0` rather than 100). Introduce `http` config settings to Azure storage. This motivated moving `s3.HTTPConfig` into a new `pkg/storage/bucket/config` package, to allow `azure` and `s3` to share it. Also update the instructions for running the website to include installing `embedmd`. Signed-off-by: Andrew Seigner <andrew@sig.gy> * feedback: `config.HTTP` -> `http.Config` also back out changelog cleanup Signed-off-by: Andrew Seigner <andrew@sig.gy> * Back out accidental changelog addition Signed-off-by: Andrew Seigner <andrew@sig.gy> Signed-off-by: Alvin Lin <alvinlin@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cortex v1.11.0 included thanos-io/thanos#3970, which added configuration
options to Azure's http client and transport, replacing usage of
http.DefaultClient. Unfortunately since Cortex was not setting thisconfig, Cortex implicitly switched from
http.DefaultClientto allempty values (e.g.
MaxIdleConns: 0rather than 100).Introduce
httpconfig settings to Azure storage. This motivated movings3.HTTPConfiginto a newpkg/storage/bucket/configpackage, to allowazureands3to share it.Also update the instructions for running the website to include
installing
embedmd.Signed-off-by: Andrew Seigner andrew@sig.gy
What this PR does:
Introduces a new
httpconfig for Azure storage, similar tos3.Which issue(s) this PR fixes:
Relates to:
With these new http defaults, we observed a 10%~40% memory reduction in alertmanager, compactor, and ruler, and more stable memory usage:
Note the above measurements assume thanos-io/thanos#4928 is merged into Cortex. That change provides a much larger memory reduction, and without it, the memory savings from this PR is negligible.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]