Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.08k stars 1.2k forks source link

Export KnownStorageErrorCode #24945

Open selimb opened 1 year ago

selimb commented 1 year ago

Is your feature request related to a problem? Please describe. I would like the KnownStorageErrorCode enum to be exported, such that error codes can more safely be checked against.

Specifically, I re-implement ContainerClient.createIfNotExists using the following because I don't like that createIfNotExists logs an error if the container already exists:

  public async ensureContainerExists(containerClient: ContainerClient) {
    // Using `containerClient.createIfNotExists` results in errors in the logs
    const exists = await containerClient.exists();
    if (exists) {
      return;
    }

    try {
      await containerClient.create();
    } catch (error) {
      if (error instanceof RestError && error.code === "ContainerAlreadyExists") {
        return;
      }
      throw error;
    }
  }

Describe the solution you'd like I would like to be able to do:

      if (error instanceof RestError && error.code === KnownStorageErrorCode.ContainerAlreadyExists) {

Describe alternatives you've considered My current workaround is to "hard-code" the ContainerAlreadyExists string in my code, as in the first snippet above.

xirzec commented 1 year ago

@selimb can you clarify what you mean by the method "logging an error"? Is it that the OpenTelemetry span is being marked as failing?

I feel like we shouldn't be marking the span as "Error" here, since as you mention this operation "succeeded" trivially:

https://github.com/Azure/azure-sdk-for-js/blob/400de15e02c40fe2bc3520545bb0188b6c9a7e0b/sdk/storage/storage-blob/src/ContainerClient.ts#L834

selimb commented 1 year ago

We're on datadog but not using OpenTelemetry yet. There's 2 things:

  1. I instrument @azure/logger such that its logs are sent through "mine". See the arrow in red below.
  2. Datadog's auto-instrumentation instruments http, and responses with an "error" status code (specifically in the range [400, 500)) are marked as failing. See arrow in blue below.

image

Note that this was just an example though, the main request was just to expose the error codes. I can imagine the error codes would be useful in other scenarios.

xirzec commented 1 year ago

Interesting! I wonder if this mechanism could lead to some other false positives like when services use CAE and Identity has to request a new access token.

But yes I'm not opposed to us better documenting/exposing the known code strings.