Implemented Feedback

aws · Nov 4, 2020 · 20e1c8c · 20e1c8c
1 parent 69e74e7
commit 20e1c8c
Show file tree

Hide file tree

Showing 20 changed files with 86 additions and 56 deletions.
diff --git a/aws/config.go b/aws/config.go
@@ -52,6 +52,11 @@ type Config struct {
 	Logger logging.Logger
 
 	// Configures the events that will be sent to the configured logger.
+	// This can be used to configure the logging of signing, retries, request, and responses
+	// of the SDK clients.
+	//
+	// See the ClientLogMode type documentation for the complete set of logging modes and available
+	// configuration.
 	ClientLogMode ClientLogMode
 }
 

diff --git a/aws/logging.go b/aws/logging.go
diff --git a/aws/logging_generate.go b/aws/logging_generate.go
@@ -11,7 +11,7 @@ import (
 var config = struct {
 	ModeBits []string
 }{
-	// Items should be appended only to keep bit-flag positions table
+	// Items should be appended only to keep bit-flag positions stable
 	ModeBits: []string{
 		"Signing",
 		"Retries",
@@ -33,8 +33,11 @@ package aws
 // each bit is a flag that describes the logging behavior for one or more client components.
 // The entire 64-bit group is reserved for later expansion by the SDK.
 //
-// Example
+// Example: Setting ClientLogMode to enable logging of retries and requests
 //  clientLogMode := aws.LogRetries | aws.LogRequest
+//
+// Example: Adding an additional log mode to an existing ClientLogMode value
+//  clientLogMode |= aws.LogResponse
 type ClientLogMode uint64
 
 // Supported ClientLogMode bits that can be configured to toggle logging of specific SDK events.
@@ -45,12 +48,14 @@ const (
 )
 
 {{ range $_, $field := .ModeBits }}
+// Is{{- $field }} returns whether the {{ $field }} logging mode bit is set
 func (m ClientLogMode) Is{{- $field }}() bool {
 	return m&{{- (symbolName $field) }} != 0
 }
 {{ end }}
 
 {{ range $_, $field := .ModeBits }}
+// Clear{{- $field }} clears the {{ $field }} logging mode bit
 func (m *ClientLogMode) Clear{{- $field }}() {
 	*m &^= {{- (symbolName $field) }}
 }

diff --git a/aws/retry/middleware.go b/aws/retry/middleware.go
@@ -31,6 +31,8 @@ type retryMetadataKey struct{}
 // AttemptMiddleware is a Smithy FinalizeMiddleware that handles retry attempts using the provided
 // Retryer implementation
 type AttemptMiddleware struct {
+	// Enable the logging of retry attempts performed by the SDK.
+	// This will include logging retry attempts, unretryable errors, and when max attempts are reached.
 	LogAttempts bool
 
 	retryer       Retryer
@@ -51,6 +53,13 @@ func (r AttemptMiddleware) ID() string {
 	return "RetryAttemptMiddleware"
 }
 
+func (r AttemptMiddleware) logf(logger logging.Logger, classification logging.Classification, format string, v ...interface{}) {
+	if !r.LogAttempts {
+		return
+	}
+	logger.Logf(classification, format, v...)
+}
+
 // HandleFinalize utilizes the provider Retryer implementation to attempt retries over the next handler
 func (r AttemptMiddleware) HandleFinalize(ctx context.Context, in smithymiddle.FinalizeInput, next smithymiddle.FinalizeHandler) (
 	out smithymiddle.FinalizeOutput, metadata smithymiddle.Metadata, err error,
@@ -85,24 +94,27 @@ func (r AttemptMiddleware) HandleFinalize(ctx context.Context, in smithymiddle.F
 				}
 			}
 
-			if r.LogAttempts {
-				logger.Logf(logging.Debug, "retrying request %s/%s, attempt %d", service, operation, attemptNum)
-			}
+			r.logf(logger, logging.Debug, "retrying request %s/%s, attempt %d", service, operation, attemptNum)
 		}
 
 		out, metadata, reqErr := next.HandleFinalize(attemptCtx, attemptInput)
 
-		relRetryToken(reqErr)
+		if releaseError := relRetryToken(reqErr); releaseError != nil && reqErr != nil {
+			return out, metadata, fmt.Errorf("failed to release token after request error, %v", reqErr)
+		}
+
 		if reqErr == nil {
 			return out, metadata, nil
 		}
 
 		retryable := r.retryer.IsErrorRetryable(reqErr)
 		if !retryable {
+			r.logf(logger, logging.Debug, "request failed with unretryable error %v", reqErr)
 			return out, metadata, reqErr
 		}
 
 		if maxAttempts > 0 && attemptNum >= maxAttempts {
+			r.logf(logger, logging.Debug, "max retry attempts exhausted, max %d", maxAttempts)
 			err = &MaxAttemptsError{
 				Attempt: attemptNum,
 				Err:     reqErr,
@@ -192,7 +204,10 @@ func setRetryMetadata(ctx context.Context, metadata retryMetadata) context.Conte
 // AddRetryMiddlewaresOptions is the set of options that can be passed to AddRetryMiddlewares for configuring retry
 // associated middleware.
 type AddRetryMiddlewaresOptions struct {
-	Retryer          Retryer
+	Retryer Retryer
+
+	// Enable the logging of retry attempts performed by the SDK.
+	// This will include logging retry attempts, unretryable errors, and when max attempts are reached.
 	LogRetryAttempts bool
 }
 

diff --git a/aws/signer/v4/v4.go b/aws/signer/v4/v4.go
@@ -94,10 +94,12 @@ type Signer struct {
 	// http://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html
 	DisableURIPathEscaping bool
 
-	// The logger to send log message to
+	// The logger to send log messages to.
 	Logger logging.Logger
 
-	// Enable logging of signed requests
+	// Enable logging of signed requests.
+	// This will enable logging of the canonical request, the string to sign, and for presigning the subsequent
+	// presigned URL.
 	LogSigning bool
 
 	keyDerivator keyDerivator
@@ -238,13 +240,8 @@ func buildAuthorizationHeader(credentialStr, signedHeadersStr, signingSignature
 	return parts.String()
 }
 
-func (v4 Signer) getLogger(ctx context.Context) (logger logging.Logger) {
-	if v4.Logger == nil {
-		logger = logging.Noop{}
-	} else {
-		logger = logging.WithContext(ctx, v4.Logger)
-	}
-	return logger
+func (v4 Signer) getLogger(ctx context.Context) logging.Logger {
+	return logging.WithContext(ctx, v4.Logger)
 }
 
 // SignHTTP signs AWS v4 requests with the provided payload hash, service name, region the

diff --git a/config/go.mod b/config/go.mod
@@ -7,7 +7,7 @@ require (
 	github.com/aws/aws-sdk-go-v2/credentials v0.1.4
 	github.com/aws/aws-sdk-go-v2/ec2imds v0.1.4
 	github.com/aws/aws-sdk-go-v2/service/sts v0.29.0
-	github.com/awslabs/smithy-go v0.3.0
+	github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2
 )
 
 replace (

diff --git a/config/go.sum b/config/go.sum
@@ -1,5 +1,5 @@
-github.com/awslabs/smithy-go v0.3.0 h1:I1EQ1P+VtxpuNnGYymATewaKrlnaYQwFvO8lNTsafbs=
-github.com/awslabs/smithy-go v0.3.0/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2 h1:Vr3nPFAbXbawQ1cJ+o9NR0UJJnMH0dbYHnwC6yKH9B8=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/google/go-cmp v0.4.1 h1:/exdXoGamhu5ONeUJH0deniYLWYvQwW66yvlfiiKTu0=
 github.com/google/go-cmp v0.4.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=

diff --git a/config/provider.go b/config/provider.go
@@ -580,24 +580,24 @@ func getClientLogMode(configs configs) (m aws.ClientLogMode, found bool, err err
 	return m, found, err
 }
 
-// LogConfigurationErrorsProvider is an interface for retrieving a boolean indicating whether configuration issues should
+// LogConfigurationWarningsProvider is an interface for retrieving a boolean indicating whether configuration issues should
 // be logged when encountered when loading from config sources.
-type LogConfigurationErrorsProvider interface {
-	GetLogConfigurationErrors() (bool, bool, error)
+type LogConfigurationWarningsProvider interface {
+	GetLogConfigurationWarnings() (bool, bool, error)
 }
 
-// WithLogConfigurationErrors implements a LogConfigurationErrorsProvider and returns the wrapped boolean value.
-type WithLogConfigurationErrors bool
+// WithLogConfigurationWarnings implements a LogConfigurationWarningsProvider and returns the wrapped boolean value.
+type WithLogConfigurationWarnings bool
 
-// GetLogConfigurationErrors returns the wrapped boolean.
-func (w WithLogConfigurationErrors) GetLogConfigurationErrors() (bool, bool, error) {
+// GetLogConfigurationWarnings returns the wrapped boolean.
+func (w WithLogConfigurationWarnings) GetLogConfigurationWarnings() (bool, bool, error) {
 	return bool(w), true, nil
 }
 
-func getLogConfigurationErrors(configs configs) (v bool, found bool, err error) {
+func getLogConfigurationWarnings(configs configs) (v bool, found bool, err error) {
 	for _, c := range configs {
-		if p, ok := c.(LogConfigurationErrorsProvider); ok {
-			v, found, err = p.GetLogConfigurationErrors()
+		if p, ok := c.(LogConfigurationWarningsProvider); ok {
+			v, found, err = p.GetLogConfigurationWarnings()
 			if err != nil {
 				return false, false, err
 			}

diff --git a/config/resolve_test.go b/config/resolve_test.go
@@ -184,7 +184,7 @@ func TestDefaultRegion(t *testing.T) {
 
 func TestResolveLogger(t *testing.T) {
 	configs := configs{
-		WithLogger(logging.Noop{}),
+		WithLogger(logging.Nop{}),
 	}
 
 	cfg := unit.Config()
@@ -194,7 +194,7 @@ func TestResolveLogger(t *testing.T) {
 		t.Fatalf("expect no error, got %v", err)
 	}
 
-	_, ok := cfg.Logger.(logging.Noop)
+	_, ok := cfg.Logger.(logging.Nop)
 	if !ok {
 		t.Error("unexpected logger type")
 	}

diff --git a/credentials/go.mod b/credentials/go.mod
@@ -6,7 +6,7 @@ require (
 	github.com/aws/aws-sdk-go-v2 v0.29.0
 	github.com/aws/aws-sdk-go-v2/ec2imds v0.1.4
 	github.com/aws/aws-sdk-go-v2/service/sts v0.29.0
-	github.com/awslabs/smithy-go v0.3.0
+	github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2
 )
 
 replace (

diff --git a/credentials/go.sum b/credentials/go.sum
@@ -1,5 +1,5 @@
-github.com/awslabs/smithy-go v0.3.0 h1:I1EQ1P+VtxpuNnGYymATewaKrlnaYQwFvO8lNTsafbs=
-github.com/awslabs/smithy-go v0.3.0/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2 h1:Vr3nPFAbXbawQ1cJ+o9NR0UJJnMH0dbYHnwC6yKH9B8=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
 github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/google/go-cmp v0.4.1 h1:/exdXoGamhu5ONeUJH0deniYLWYvQwW66yvlfiiKTu0=

diff --git a/ec2imds/go.mod b/ec2imds/go.mod
@@ -4,7 +4,7 @@ go 1.15
 
 require (
 	github.com/aws/aws-sdk-go-v2 v0.29.0
-	github.com/awslabs/smithy-go v0.3.0
+	github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2
 	github.com/google/go-cmp v0.4.1
 )
 

diff --git a/ec2imds/go.sum b/ec2imds/go.sum
@@ -1,5 +1,5 @@
-github.com/awslabs/smithy-go v0.3.0 h1:I1EQ1P+VtxpuNnGYymATewaKrlnaYQwFvO8lNTsafbs=
-github.com/awslabs/smithy-go v0.3.0/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2 h1:Vr3nPFAbXbawQ1cJ+o9NR0UJJnMH0dbYHnwC6yKH9B8=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/google/go-cmp v0.4.1 h1:/exdXoGamhu5ONeUJH0deniYLWYvQwW66yvlfiiKTu0=
 github.com/google/go-cmp v0.4.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=

diff --git a/example/service/s3/listObjects/go.mod b/example/service/s3/listObjects/go.mod
@@ -4,12 +4,8 @@ go 1.15
 
 require (
 	github.com/aws/aws-sdk-go-v2/config v0.2.2
+	github.com/aws/aws-sdk-go-v2/service/internal/s3shared v0.3.1 // indirect
 	github.com/aws/aws-sdk-go-v2/service/s3 v0.29.0
-	github.com/aws/aws-sdk-go-v2 v0.29.0
-	github.com/aws/aws-sdk-go-v2/service/sts v0.29.0
-	github.com/aws/aws-sdk-go-v2/credentials v0.1.4
-	github.com/aws/aws-sdk-go-v2/ec2imds v0.1.4
-	github.com/aws/aws-sdk-go-v2/service/internal/s3shared v0.3.1
 )
 
 replace github.com/aws/aws-sdk-go-v2/config => ../../../../config/

diff --git a/example/service/s3/listObjects/go.sum b/example/service/s3/listObjects/go.sum
@@ -1,5 +1,7 @@
 github.com/awslabs/smithy-go v0.3.0 h1:I1EQ1P+VtxpuNnGYymATewaKrlnaYQwFvO8lNTsafbs=
 github.com/awslabs/smithy-go v0.3.0/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2 h1:Vr3nPFAbXbawQ1cJ+o9NR0UJJnMH0dbYHnwC6yKH9B8=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/google/go-cmp v0.4.1 h1:/exdXoGamhu5ONeUJH0deniYLWYvQwW66yvlfiiKTu0=
 github.com/google/go-cmp v0.4.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=

diff --git a/feature/s3/manager/download.go b/feature/s3/manager/download.go
@@ -174,11 +174,7 @@ func (d Downloader) Download(ctx context.Context, w io.WriterAt, input *s3.GetOb
 	}
 
 	// Ensures we don't need nil checks later on
-	if impl.cfg.Logger == nil {
-		impl.cfg.Logger = logging.Noop{}
-	} else {
-		impl.cfg.Logger = logging.WithContext(ctx, impl.cfg.Logger)
-	}
+	impl.cfg.Logger = logging.WithContext(ctx, impl.cfg.Logger)
 
 	impl.partBodyMaxRetries = d.PartBodyMaxRetries
 

diff --git a/feature/s3/manager/go.mod b/feature/s3/manager/go.mod
@@ -5,13 +5,10 @@ go 1.15
 require (
 	github.com/aws/aws-sdk-go-v2 v0.29.0
 	github.com/aws/aws-sdk-go-v2/config v0.2.2
+	github.com/aws/aws-sdk-go-v2/service/internal/s3shared v0.3.1 // indirect
 	github.com/aws/aws-sdk-go-v2/service/s3 v0.29.0
-	github.com/awslabs/smithy-go v0.3.0
+	github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2
 	github.com/google/go-cmp v0.4.1
-	github.com/aws/aws-sdk-go-v2/service/sts v0.29.0
-	github.com/aws/aws-sdk-go-v2/credentials v0.1.4
-	github.com/aws/aws-sdk-go-v2/ec2imds v0.1.4
-	github.com/aws/aws-sdk-go-v2/service/internal/s3shared v0.3.1
 )
 
 replace github.com/aws/aws-sdk-go-v2 => ../../../

diff --git a/feature/s3/manager/go.sum b/feature/s3/manager/go.sum
@@ -1,5 +1,7 @@
 github.com/awslabs/smithy-go v0.3.0 h1:I1EQ1P+VtxpuNnGYymATewaKrlnaYQwFvO8lNTsafbs=
 github.com/awslabs/smithy-go v0.3.0/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2 h1:Vr3nPFAbXbawQ1cJ+o9NR0UJJnMH0dbYHnwC6yKH9B8=
+github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2/go.mod h1:hPOQwnmBLHsUphH13tVSjQhTAFma0/0XoZGbBcOuABI=
 github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/google/go-cmp v0.4.1 h1:/exdXoGamhu5ONeUJH0deniYLWYvQwW66yvlfiiKTu0=

diff --git a/go.mod b/go.mod
@@ -1,7 +1,7 @@
 module github.com/aws/aws-sdk-go-v2
 
 require (
-	github.com/awslabs/smithy-go v0.3.0
+	github.com/awslabs/smithy-go v0.3.1-0.20201104001100-c2a3078f61a2
 	github.com/google/go-cmp v0.4.1
 	github.com/jmespath/go-jmespath v0.4.0
 )