aws / aws-sdk-js-v3

Modularized AWS SDK for JavaScript.
Apache License 2.0
3.03k stars 567 forks source link

cloudwatch-logs is not aware of `ThrottlingException` #5140

Closed mrgrain closed 2 months ago

mrgrain commented 1 year ago

Checkboxes for prior research

Describe the bug

According to the code and docs, commands in the cloudwatch-logs package do not throw a ThrottlingException.

image

Evidence 1: Other packages list ThrottlingException here.

image

Evidence 2: Other packages include a ThrottlingException class.

However in reality cloudwatch-logs commands can throw a ThrottlingException:

image

Evidence 3: The createLogGroupSafe function is a thin wrapper around CreateLogGroupCommand.

This is an issue because I cannot catch a ThrottlingException from this package in the recommended way if (error instanceof Logs.ThrottlingException) { /* ... */ } because the package does not export the exception type.

SDK version number

@aws-sdk/client-cloudwatch-logs@3.398.0

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

Lambda Runtime nodejs18.x

Reproduction Steps

import { CloudWatchLogsClient, CreateLogGroupCommand } from "@aws-sdk/client-cloudwatch-logs";

const numberOfLogGroups = 200;
const random = (Math.random() + 1).toString(36).substring(7);
const logGroupNames = [...Array(numberOfLogGroups).keys()].map(i => `/reproduction/${random}/${i}`);

const client = new CloudWatchLogsClient({
    maxAttempts: 1 // makes it more likely to hit the error
});

logGroupNames.forEach(async (logGroupName) => {
    try {
        const params = { logGroupName };
        const command = new CreateLogGroupCommand(params);
        await client.send(command);
    } catch (error: any) {
        if (error instanceof ThrottlingException) { // ThrottlingException class does not exist
        // if (error.name === 'ThrottlingException') { // This does work
            console.log("CAUGHT a wild ThrottlingException")
            return;
        }

        throw error;
    }
});

Observed Behavior

There is no class/type to use with instanceof

Expected Behavior

There is a class/type to use with instanceof JSDocs and Doc website state that a ThrottlingException might be thrown

Possible Solution

No response

Additional Information/Context

No response

RanVaknin commented 1 year ago

Hi @mrgrain ,

Thanks for opening this thread. What you are witnessing is a discrepancy between Server behavior and API model definition.

You can take a look at the internal model and witness that a ThrottlingException is in fact not modeled.

If you didn't know, all the AWS SDKs are code generated from the internal models defined by service teams. So simply put, if a service doesn't specify a certain error in their API model, it wouldn't exist in the SDKs.

Since ThrottlingException is sometimes considered a base exception and not every service explicitly models it, it does exist in the smithy-typscript package. So I think you can actually import it like so:

import { CloudWatchLogsClient, CreateLogGroupCommand } from "@aws-sdk/client-cloudwatch-logs";
import { ThrottlingException } from '@smithy/service-error-classification';

const numberOfLogGroups = 200;
const random = (Math.random() + 1).toString(36).substring(7);
const logGroupNames = [...Array(numberOfLogGroups).keys()].map(i => `/reproduction/${random}/${i}`);

const client = new CloudWatchLogsClient({
    maxAttempts: 1 // makes it more likely to hit the error
});

logGroupNames.forEach(async (logGroupName) => {
    try {
        const params = { logGroupName };
        const command = new CreateLogGroupCommand(params);
        await client.send(command);
    } catch (error: any) {
        if (error instanceof ThrottlingException) { 
            console.log("CAUGHT a wild ThrottlingException")
            return;
        }

        throw error;
    }
});

At any rate, I think its safe to error handle this specific error with .name instance of instanceof. If you feel inclined, you can create an internal feature reqeust via the AWS console and ask to be routed to the cloudwatch-logs service team.

Thanks again, Ran~

github-actions[bot] commented 1 year ago

This issue has not received a response in 1 week. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.

rix0rrr commented 1 year ago

Hi @RanVaknin,

I believe that you have a reason for why you have defined the behavior as it currently is. The problem is that now there is a discrepancy between "modeled exception handling" and "unmodeled exception handling", that didn't need to be there. The SDK v2 didn't have a bimodal error handling pattern, but the SDK v3 is now recommending this as a best practice.

I understand that your model is more pure, but it is putting a burden on your users that didn't need to be there. That can't be your intention, can it?

Example:

SDKv2

try {
  new Logs().createLogGroup({ ... });
} catch(e) {
  switch (e.code) {
    // Handling all exceptions looks exactly the same, regardless of what the service team did

    case 'ResourceAlreadyExistsException': 
      // Handle  ResourceAlreadyExistsException
      break;
    case 'ThrottlingException':
      // Handle  ResourceAlreadyExistsException
      break; 
    default: 
      // ...
      break;
  }
}

SDKv3

try {
  new CloudWatchLogsClient().send(new CreateLogGroupCommand({ ... });
} catch(e) {
  // Handle some exceptions with instanceof, others with a name check

  if (e instanceof ResourceAlreadyExistsException) {
      // Handle  ResourceAlreadyExistsException
  } else if (e.name === 'ResourceAlreadyExistsException') {
      // Handle  ResourceAlreadyExistsException
  } else {
     // ...
  }
}

Where I used to be able to just rely on the Errors section of the AWS API documentation to get my code right, I now need to look both at the Errors section, as well as the service model to guess what code I need to write.

Would you not agree this is a more brittle situation?

I think your response to this is: "the service team should do a better job of describing all their errors". But isn't a design in which you do NOT have to rely on the service team's effort a strictly better solution?

albertodiazdorado commented 10 months ago

I think that @rix0rrr is right. I think that nothing will change in the SDK.

I have already installed @smithy/service-error-classification and I am using it, since I have no hope of any other solution.

Peace.

// ThrottlingException is not exposed
import { isThrottlingError } from "@smithy/service-error-classification";

// ...

if (error instanceof Error && isThrottlingError(error)) {
  // ...
}
RanVaknin commented 2 months ago

Hi there,

I understand the frustration here. The core issue is that certain services are returning un-modeled exception, therefore you cannot use instanceof on those. To circumvent that you can use .name which is not the recommended way, but it should safe.

The SDK team cannot introduce new error types to the SDK client since the SDK is code generated from the model of that upstream AWS service the SDK interacts with. This is what ensures type safety. Unless the service team adds ThrottlingException to their API model, your only recourse in this case is to rely on the raw string.

Since this is not actionable by the SDK team, Im going to go ahead and close this. Thanks, Ran~

github-actions[bot] commented 1 month ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.