[@opentelemetry/instrumentation-aws-sdk] Allow to ignore specific exception errors

project0 commented 8 months ago

Is your feature request related to a problem? Please describe

The AWS SDK v3 is raising a "NoSuchKey" exception when a a object is not found. This is expected and how the API works.

Unfortunately it also creates a error span in the trace, as exceptions seem to be catched in the AWS instrumentation library. This creates unnecessarily false positive as i see errors what i do not consider a error.

Technically this is correct, but usually it is expected to retrieve this exception and will be catched by the application to handle this case. So depending on the use case it is either a error or a way to indicate a object exists.

Describe the solution you'd like to see

I see two solutions:

Either a simple config flag to suppress specific exceptions
a ignore hook function to filter such things.

Describe alternatives you've considered

I would be glad if someone could give me a hint how to solve it.

Additional context

aws sdk v3

project0 commented 8 months ago

:disappointed: I also tried to override the status with responseHook, but apparently it is never called as the exception handler kicks in before.

justinedelson commented 5 months ago

I have a very similar issue to what @project0 describes above... we detect this type of error in application code and having the error surfaced in the span status creates troublesome noise that makes actual errors harder to uncover. While it is possible to suppress tracing entirely from a wrapping span by using the same technique that the AWS SDK instrumentation uses here: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/78953064f8bd957649b8052b03debb200784b351/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts#L696, I didn't want to lose the span which produces the error entirely, just change whether or not the span status was set.

The solution I have been using (which required downstream patching of @opentelemetry/instrumentation-aws-sdk is as follows:

In the instrumentation for the library that uses the AWS SDK, we set a context value to an array of command names:
```
api.context.active().setValue(COMMANDS_TO_SUPPRESS_ERRORS_KEY, ['HeadObject'])
```
In the patched AWS instrumentation, when an error occurs, if the context value exists, the span status is not set and the error is stored as a span attribute:

const commandsToSuppressErrors = api.context.active().getValue(COMMANDS_TO_SUPPRESS_ERRORS_KEY);
const suppressError = commandsToSuppressErrors && commandsToSuppressErrors.includes(normalizedRequest.commandName);

if (suppressError) {
    span.setAttribute('error.suppressed', err.message || 'true');
} else {
    // this is the original code
    span.setStatus({
        code: api.SpanStatusCode.ERROR,
        message: err.message,
    });
};

I would be happy to submit a pull request for the same. The only doubt I have is whether the context key (in the snippets above the constant COMMANDS_TO_SUPPRESS_ERRORS_KEY) should be defined by the AWS SDK instrumentation or provided in config. Either way would work for me. @project0 do you have a perspective on this?

project0 commented 5 months ago

I am not sure if commands is enough, personally i would prefer something like the responseHook, so i can fine grand my needs based on the request/response.

earonesty commented 2 months ago

honestly, there needs to be a way to list exception classes to not-log-as-errors. throwing exceptions for status is normal behavior for certain functions (think raise NotCachedError) especially in js/python where exception tossing is considered good behavior

open-telemetry / opentelemetry-js-contrib