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

otelaws s3 error responses should not always be recorded #6331

Open
sjones4 opened this issue Nov 14, 2024 · 1 comment
Open

otelaws s3 error responses should not always be recorded #6331

sjones4 opened this issue Nov 14, 2024 · 1 comment
Labels
area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelaws

Comments

@sjones4
Copy link

sjones4 commented Nov 14, 2024

Description

Instrumentation for s3 records 3xx http responses as errors.

https://github.com/open-telemetry/opentelemetry-go-contrib/blob/11cf0df7388a/instrumentation/github.com/aws/aws-sdk-go-v2/otelaws/aws.go#L75-L79

There is already http.status_code that covers the result of the call, so recording the error does not add any useful information.

When getting objects from s3 conditionally a 3xx http response is expected if the condition is not satisfied. For example, you might get an object if is has been modified since the last time it was accessed.

Environment

  • OS: Linux
  • Architecture: x86
  • Go Version: 1.23.2
  • otelaws version: v0.52.0

Steps To Reproduce

s3Client.GetObject(ctx, &s3.GetObjectInput{
		Bucket:          aws.String(bucket),
		Key:             aws.String(object),
		IfModifiedSince: lastModified,
	})

Expected behavior

Don't record an error for 3xx responses.

Potential fix

The error could be checked using something like:

import (
	"net/http"
	awshttp "github.com/aws/aws-sdk-go-v2/aws/transport/http"

...

	var respErr *awshttp.ResponseError
	if errors.As(err, &respErr) && respErr.HTTPStatusCode() == http.StatusNotModified {

It seems reasonable to do this for all services, or there could be an "IsTraceableError" check for each service which would not record these "error" responses.

I can create a PR for this if useful.

@sjones4 sjones4 added area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelaws labels Nov 14, 2024
@dmathieu
Copy link
Member

cc @akats7 as code owner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelaws
Projects
None yet
Development

No branches or pull requests

2 participants