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.

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

Introduce http config settings in Azure storage #4581

Merged
merged 3 commits into from
Dec 13, 2021
Merged

Introduce http config settings in Azure storage #4581

merged 3 commits into from
Dec 13, 2021

Conversation

siggy
Copy link
Contributor

@siggy siggy commented Dec 8, 2021

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 [email protected]

What this PR does:

Introduces a new http config for Azure storage, similar to s3.

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:

cortex http config

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

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

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 <[email protected]>
@siggy siggy marked this pull request as ready for review December 9, 2021 00:33
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Thanks! Couple of thoughts below.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated change - please move to a different PR.

Copy link
Contributor Author

@siggy siggy Dec 9, 2021

Choose a reason for hiding this comment

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

Posted #4584, will remove from here.

@@ -0,0 +1,35 @@
package config
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@siggy siggy Dec 9, 2021

Choose a reason for hiding this comment

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

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 <[email protected]>
@siggy siggy mentioned this pull request Dec 9, 2021
3 tasks
Copy link
Member

@alvinlin123 alvinlin123 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@alvinlin123 alvinlin123 merged commit a78083d into cortexproject:master Dec 13, 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 <[email protected]>

* feedback: `config.HTTP` -> `http.Config`

also back out changelog cleanup

Signed-off-by: Andrew Seigner <[email protected]>

* Back out accidental changelog addition

Signed-off-by: Andrew Seigner <[email protected]>
Signed-off-by: Alvin Lin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants