fix(otelaws): don't record 304 Not Modified as a span error

CHANGELOG.md

@@ -17,6 +17,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Report `ot-baggage-*` extraction errors from `go.opentelemetry.io/contrib/propagators/ot` to `otel.Handle` instead of silently discarding them, while still attaching the successfully parsed baggage members to the context. (#9395)
 - Set `error.type` on the `rpc.client.call.duration` and `rpc.server.call.duration` metrics in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` when the RPC fails with a non-OK status, per the RPC semantic conventions. (#9429)
 - Reject OTLP exporter headers with an empty `name` in `go.opentelemetry.io/contrib/otelconf`, `go.opentelemetry.io/contrib/otelconf/x`, and `go.opentelemetry.io/contrib/otelconf/v0.3.0`, instead of forwarding invalid header names to OTLP exporters. (#9102)
+- Don't mark spans as errors in `go.opentelemetry.io/contrib/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws` when the AWS SDK returns an error wrapping an HTTP `304 Not Modified` response, such as from a conditional S3 `GetObject` request whose precondition (e.g. `IfModifiedSince`) was not satisfied. (#9474)
 
 <!-- Released section -->
 <!-- Don't change this section unless doing release -->

instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go

@@ -6,6 +6,8 @@ package otelaws
 
 import (
 	"context"
+	"errors"
+	"net/http"
 	"time"
 
 	v2Middleware "github.com/aws/aws-sdk-go-v2/aws/middleware"
@@ -71,7 +73,7 @@ func (m otelMiddlewares) initializeMiddlewareAfter(stack *middleware.Stack) erro
 
 		out, metadata, err = next.HandleInitialize(ctx, in)
 		span.SetAttributes(m.buildAttributes(ctx, in, out)...)
-		if err != nil {
+		if err != nil && !isNotModifiedStatus(err) {
 			span.SetAttributes(semconv.ErrorType(err))
 			span.SetStatus(codes.Error, err.Error())
 		}
@@ -131,6 +133,28 @@ func (m otelMiddlewares) buildAttributes(ctx context.Context, in middleware.Init
 	return attributes
 }
 
+// responseStatusCode reports the HTTP status code carried by err, if err
+// wraps a *smithyhttp.ResponseError.
+func responseStatusCode(err error) (code int, ok bool) {
+	var respErr *smithyhttp.ResponseError
+	if !errors.As(err, &respErr) {
+		return 0, false
+	}
+	return respErr.HTTPStatusCode(), true
+}
+
+// isNotModifiedStatus reports whether err wraps an HTTP response error with
+// a 304 Not Modified status code. This is returned, for example, by a
+// conditional S3 GetObject request whose precondition (such as
+// IfModifiedSince) was not satisfied. It indicates the precondition was not
+// met, not a failure of the request, and should not be recorded as a span
+// error. Other 3xx status codes (e.g. a 301 signaling a misconfigured
+// bucket region) can indicate a real problem and are still recorded.
+func isNotModifiedStatus(err error) bool {
+	code, ok := responseStatusCode(err)
+	return ok && code == http.StatusNotModified
+}
+
 func spanName(serviceID, operation string) string {
 	spanName := serviceID
 	if operation != "" {

instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws_test.go

@@ -5,6 +5,7 @@ package otelaws
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"testing"
 	"time"
@@ -172,6 +173,49 @@ func Test_otelMiddlewares_presignedRequests(t *testing.T) {
 	assert.NotContains(t, input.Header[key], value)
 }
 
+func Test_isNotModifiedStatus(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{
+			name: "nil error",
+			err:  nil,
+			want: false,
+		},
+		{
+			name: "non-response error",
+			err:  errors.New("boom"),
+			want: false,
+		},
+		{
+			name: "304 response error",
+			err: &smithyhttp.ResponseError{
+				Response: &smithyhttp.Response{
+					Response: &http.Response{StatusCode: http.StatusNotModified},
+				},
+			},
+			want: true,
+		},
+		{
+			name: "301 response error",
+			err: &smithyhttp.ResponseError{
+				Response: &smithyhttp.Response{
+					Response: &http.Response{StatusCode: http.StatusMovedPermanently},
+				},
+			},
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, isNotModifiedStatus(tt.err))
+		})
+	}
+}
+
 func Test_Span_name(t *testing.T) {
 	serviceID1 := ""
 	serviceID2 := "ServiceID"

instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/awstest_test.go

@@ -39,6 +39,7 @@ func TestAppendMiddlewares(t *testing.T) {
 		responseBody       []byte
 		expectedRegion     string
 		expectedError      codes.Code
+		expectSDKError     bool
 		expectedRequestID  string
 		expectedStatusCode int
 	}{
@@ -53,6 +54,7 @@ func TestAppendMiddlewares(t *testing.T) {
 		</InvalidChangeBatch>`),
 			expectedRegion:     "us-east-1",
 			expectedError:      codes.Error,
+			expectSDKError:     true,
 			expectedRequestID:  "b25f48e8-84fd-11e6-80d9-574e0c4664cb",
 			expectedStatusCode: http.StatusInternalServerError,
 		},
@@ -71,6 +73,7 @@ func TestAppendMiddlewares(t *testing.T) {
 		`),
 			expectedRegion:     "us-west-1",
 			expectedError:      codes.Error,
+			expectSDKError:     true,
 			expectedRequestID:  "1234567890A",
 			expectedStatusCode: http.StatusNotFound,
 		},
@@ -87,6 +90,21 @@ func TestAppendMiddlewares(t *testing.T) {
 			expectedRegion:     "us-west-2",
 			expectedStatusCode: http.StatusOK,
 		},
+
+		"not modified response is not recorded as an error": {
+			responseStatus:     http.StatusNotModified,
+			expectedRegion:     "us-west-2",
+			expectSDKError:     true,
+			expectedStatusCode: http.StatusNotModified,
+		},
+
+		"other redirect response is still recorded as an error": {
+			responseStatus:     http.StatusMovedPermanently,
+			expectedRegion:     "us-west-2",
+			expectedError:      codes.Error,
+			expectSDKError:     true,
+			expectedStatusCode: http.StatusMovedPermanently,
+		},
 	}
 
 	for name, c := range cases {
@@ -125,10 +143,10 @@ func TestAppendMiddlewares(t *testing.T) {
 					&options.APIOptions, otelaws.WithTracerProvider(provider),
 				)
 			})
-			if c.expectedError == codes.Unset {
-				assert.NoError(t, err)
-			} else {
+			if c.expectSDKError {
 				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
 			}
 
 			spans := sr.Ended()