enhancement: Expose MinIO Go retry configuration for S3

[rwhitty]

What this PR does:
Makes minio retry settings configurable via the S3 config, allowing users to customize Tempo's retry behavior for S3 requests.

Which issue(s) this PR fixes:
Fixes #6112

Checklist

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

[CLAassistant]

All committers have signed the CLA.

[knylander-grafana]

Thank you for adding doc! Do we also need a changelog entry for this?

[rwhitty]

Thank you for adding doc! Do we also need a changelog entry for this?

Yep sorry about that, added!

[rwhitty]

Hi @knylander-grafana, wanted to check if you guys had a timeline on reviewing this? Thanks!

[knylander-grafana]

Hi @knylander-grafana, wanted to check if you guys had a timeline on reviewing this? Thanks!

Greetings! I can review docs changes only. I'll ask the team to take a look.

[rwhitty]

Hi @knylander-grafana, wanted to check if you guys had a timeline on reviewing this? Thanks!

Greetings! I can review docs changes only. I'll ask the team to take a look.

Thank you!

[mdisibio]

@rwhitty Thanks for your patience, I started reviewing this last week and chatted briefly with the team, am now able to leave feedback.

First I'd like to understand a little more context - are you finding that changing these values is helping with performance/stability, hence the need to expose them?

We are good with exposing these values, but I'd like to request a few changes:

(1) naming: rather than expose the Tempo config with these names directly, I think these would work better:

RetryMaxAttempts int `yaml:"retry_max_attempts"`
... retry_backoff_initial
... retry_backoff_max

(2) In the current state there are some inconsistencies between the defaults in the manifest and in actuality. For example the manifest has retry_unit: 0s, but it's really going to run with 200ms. I think we can fix this by setting the defaults here:

func (cfg *Config) RegisterFlagsAndApplyDefaults
...
cfg.RetryMaxAttempts = minio.MaxRetry
<others>

Then the manifest should show the real defaults. I think we can leave > 0 logic. In my mind that protects against initialization with incomplete config via tests/etc, and not valid values to disable any of this (which is not required since it was never possible to disable before).

[rwhitty]

@rwhitty Thanks for your patience, I started reviewing this last week and chatted briefly with the team, am now able to leave feedback.

First I'd like to understand a little more context - are you finding that changing these values is helping with performance/stability, hence the need to expose them?

We are good with exposing these values, but I'd like to request a few changes:

(1) naming: rather than expose the Tempo config with these names directly, I think these would work better:

RetryMaxAttempts int `yaml:"retry_max_attempts"`
... retry_backoff_initial
... retry_backoff_max

(2) In the current state there are some inconsistencies between the defaults in the manifest and in actuality. For example the manifest has retry_unit: 0s, but it's really going to run with 200ms. I think we can fix this by setting the defaults here:

func (cfg *Config) RegisterFlagsAndApplyDefaults
...
cfg.RetryMaxAttempts = minio.MaxRetry
<others>

Then the manifest should show the real defaults. I think we can leave > 0 logic. In my mind that protects against initialization with incomplete config via tests/etc, and not valid values to disable any of this (which is not required since it was never possible to disable before).

Hi @mdisibio, thanks for the feedback!

For context, what you said is basically correct. I'm with Bloomberg, and exposing these settings will allow us to better tune Tempo to match our organization's best practices for S3 regarding performance/stability.

I'll push a couple commits in a minute to address the points you brought up.

[mdisibio]

exposing these settings will allow us to better tune Tempo to match our organization's best practices for S3 regarding performance/stability

Got it, would it be possible to share the values you have in mind, or summarize? This is an area we haven't benchmarked in awhile (including optimal read strip size), and it could be useful to us as well.

Sorry with the last commit it changed the whitespace of the existing struct fields and is hitting an existing lint issue. I tested locally and this should be enough to fix:

AccessKey         string         `yaml:"access_key"` // #nosec G117

Also, there is a conflict with changelog. Otherwise LGTM, can merge when ready.

[rwhitty]

Got it, would it be possible to share the values you have in mind, or summarize? This is an area we haven't benchmarked in awhile (including optimal read strip size), and it could be useful to us as well.

The default retry behavior is a little too aggressive, so we'd be increasing one or both of retry_backoff_initial and retry_backoff_max. Not 100% sure of the exact values, but it's mostly for stability reasons as opposed to performance optimization.

Sorry with the last commit it changed the whitespace of the existing struct fields and is hitting an existing lint issue.

Also, there is a conflict with changelog. Otherwise LGTM, can merge when ready.

Will fix the linting/changelog issues momentarily. Thanks again!