Skip to content

Update comments for AWS SDK logging and retryer #1303

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.

Sign up for GitHub

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

Open
radoslawszulgo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update comments for AWS SDK logging and retryer #1303

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11491a4
Update comments for AWS SDK logging and retryer
radoslawszulgo Apr 8, 2026
aa137f2
Fix formatting of Retryer field tags
radoslawszulgo Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions pbm/storage/s3/s3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,29 +61,28 @@ type Config struct {
// certificate chain and host name
InsecureSkipTLSVerify bool `bson:"insecureSkipTLSVerify" json:"insecureSkipTLSVerify" yaml:"insecureSkipTLSVerify"`

// DebugLogLevels enables AWS SDK debug logging (sub)levels. Available options:
// LogDebug, Signing, HTTPBody, RequestRetries, RequestErrors, EventStreamBody
//
// Any sub levels will enable LogDebug level accordingly to AWS SDK Go module behavior
// https://pkg.go.dev/github.com/aws/aws-sdk-go@v1.40.7/aws#LogLevelType
// DebugLogLevels enables AWS SDK logging levels. Available options:
// Signing, Retries, Request, RequestWithBody, Response, ResponseWithBody, DeprecatedUsage, RequestEventMessage

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

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

The DebugLogLevels comment lists supported values, but it omits ResponseEventMessage, which is accepted by toClientLogMode (and defined in SDKDebugLogLevel). Please update the comment to include ResponseEventMessage so configuration docs match actual behavior.

Suggested change
// Signing, Retries, Request, RequestWithBody, Response, ResponseWithBody, DeprecatedUsage, RequestEventMessage
// Signing, Retries, Request, RequestWithBody, Response, ResponseWithBody, DeprecatedUsage, RequestEventMessage, ResponseEventMessage

Copilot uses AI. Check for mistakes.
// and deprecated: LogDebug, HTTPBody, RequestRetries, RequestErrors, EventStreamBody
// Above values are translated to match the AWS SDK V2:

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

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

The comment says the listed DebugLogLevels values are "translated to match the AWS SDK V2", but the primary values listed (Signing, Retries, Request, etc.) are already the AWS SDK v2 aws.ClientLogMode flag names; only the deprecated v1 values are mapped for backward compatibility. Consider rewording to clarify that this field is parsed into aws.ClientLogMode and that deprecated v1 names are mapped to the corresponding v2 flags.

Suggested change
// Above values are translated to match the AWS SDK V2:
// Values are parsed into AWS SDK v2 ClientLogMode; deprecated names are mapped
// to the corresponding v2 flags for backward compatibility:

Copilot uses AI. Check for mistakes.
// https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#ClientLogMode
DebugLogLevels string `bson:"debugLogLevels,omitempty" json:"debugLogLevels,omitempty" yaml:"debugLogLevels,omitempty"`

// Retryer is configuration for client.DefaultRetryer
// https://pkg.go.dev/github.com/aws/aws-sdk-go/aws/client#DefaultRetryer
// Retryer is a configuration for aws.Retryer
// https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#Retryer
// it's implemented with standard retryable, and delay behavior
// see also: https://github.com/aws/aws-sdk-go-v2/blob/v1.41.5/aws/retry/standard.go
Comment on lines +71 to +74

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

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

The Retryer field comment states "it's implemented with standard retryable, and delay behavior" and links to a specific aws-sdk-go-v2 GitHub tag. In this codebase the implementation is NewCustomRetryer (wrapping retry.NewStandard and enforcing a minimum backoff) and the pinned SDK version may differ over time, so the hardcoded GitHub URL/tag can become stale. Please reword to describe the local implementation (and/or link to aws/retry docs) without tying the comment to a specific upstream tag.

Suggested change
// Retryer is a configuration for aws.Retryer
// https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#Retryer
// it's implemented with standard retryable, and delay behavior
// see also: https://github.com/aws/aws-sdk-go-v2/blob/v1.41.5/aws/retry/standard.go
// Retryer configures the aws.Retryer used by the local custom retryer implementation.
// https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws#Retryer
// The implementation wraps the AWS SDK standard retryer behavior and applies the
// configured retry delay settings, including enforcing a minimum retry delay.

Copilot uses AI. Check for mistakes.
Retryer *Retryer `bson:"retryer,omitempty" json:"retryer,omitempty" yaml:"retryer,omitempty"`
}

type Retryer struct {
// Num max Retries is the number of max retries that will be performed.
// https://pkg.go.dev/github.com/aws/aws-sdk-go/aws/client#DefaultRetryer.NumMaxRetries
// NumMaxRetries is the number of **additional** max attempts that should be made

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

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

NumMaxRetries comment uses Markdown formatting ("additional") and has an extra space after the field name. Go doc comments are rendered as plain text; please drop Markdown emphasis and keep formatting consistent (e.g., "NumMaxRetries is the number of additional retry attempts"), matching the NewCustomRetryer behavior of MaxAttempts = numMaxRetries + 1.

Suggested change
// NumMaxRetries is the number of **additional** max attempts that should be made
// NumMaxRetries is the number of additional retry attempts that should be made

Copilot uses AI. Check for mistakes.
NumMaxRetries int `bson:"numMaxRetries" json:"numMaxRetries" yaml:"numMaxRetries"`

// MinRetryDelay is the minimum retry delay after which retry will be performed.
// https://pkg.go.dev/github.com/aws/aws-sdk-go/aws/client#DefaultRetryer.MinRetryDelay
// MinRetryDelay is the minimum delay that will be returned from RetryDelay
MinRetryDelay time.Duration `bson:"minRetryDelay" json:"minRetryDelay" yaml:"minRetryDelay"`

// MaxRetryDelay is the maximum retry delay before which retry must be performed.
// https://pkg.go.dev/github.com/aws/aws-sdk-go/aws/client#DefaultRetryer.MaxRetryDelay
// MaxRetryDelay is the duration between retried attempts (using ExponentialJitterBackoff)

Copilot AI Apr 8, 2026

Copy link

Choose a reason for hiding this comment

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

The MaxRetryDelay comment is misleading: it currently reads as if this value is "the duration between retried attempts", but the implementation passes it as retry.StandardOptions.MaxBackoff (a maximum cap on the computed exponential-jitter backoff). Please update the comment to reflect that it is a maximum backoff / maximum retry delay cap, not a fixed delay between attempts.

Suggested change
// MaxRetryDelay is the duration between retried attempts (using ExponentialJitterBackoff)
// MaxRetryDelay is the maximum backoff / retry delay cap for retried attempts (using ExponentialJitterBackoff)

Copilot uses AI. Check for mistakes.
MaxRetryDelay time.Duration `bson:"maxRetryDelay" json:"maxRetryDelay" yaml:"maxRetryDelay"`
}

Expand Down
Loading