Proposal: Dapr Error Handling Codes

[robertojrojas]

This is a proposal for Dapr Error Handling and Codes standarization.
Issue: dapr/dapr#6068

[mukundansundar]

first pass review.

[mukundansundar]

I think, these should be completely platform agnostic.

[ItalyPaleAle]

Agree

[mukundansundar]

remove platform specific codes.

[ItalyPaleAle]

Would love to see an example of how this is included in a HTTP response too.

[ItalyPaleAle]

Should this be only gRPC codes? This way, developers get a consistent response with both HTTP and gRPC.
When the API is HTTP, the corresponding status code is returned as HTTP response status code.

[robertojrojas]

We could address the HTTP error codes with a mapping like the one shown in the table: https://cloud.google.com/apis/design/errors#handling_errors

[ItalyPaleAle]

How are errors returned in HTTP, in general?

[robertojrojas]

@ItalyPaleAle Here what I did as part of my POC, but of course it could implemented better: https://github.com/robertojrojas/dapr/blob/a0760ea549b9a3da93af2bee96e88ad94db9b32b/pkg/http/api.go#L1017

[robertojrojas]

@ItalyPaleAle Here is a sample HTTP output of the code above. Here you can see both the gRPC Unavailable and the HTTP 500 but that's because in my POC code I used fasthttp.StatusInternalServerError. We could have code mapping gRPC and HTTP based on the table in the handling errors page :

  curl -v -s http://localhost:3500/v1.0/state/statestore/name | jq .
dapr_redis
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 3500 (#0)
> GET /v1.0/state/statestore/name HTTP/1.1
> Host: localhost:3500
> User-Agent: curl/7.64.1
> Accept: */*
> 
< HTTP/1.1 500 Internal Server Error
< Date: Tue, 16 May 2023 16:32:49 GMT
< Content-Type: application/json
< Content-Length: 269
< Traceparent: 00-a1b43cb07328721cc009960b070586b6-4d4a624d2ab86d00-01
< 
{ [269 bytes data]
* Connection #0 to host localhost left intact
* Closing connection 0
{
  "code": 14,
  "message": "fail to get name from state store statestore: dial tcp [::1]:6379: connect: connection refused",
  "details": [
    {
      "@type": "type.googleapis.com/google.rpc.ErrorInfo",
      "reason": "DAPR_RUNTIME_HTTP",
      "domain": "dapr.io",
      "metadata": {
        "storeName": "statestore"
      }
    }
  ]
}

[ItalyPaleAle]

Thoughts on making this the same key? For example state.redis/v1 which is how we refer to components in the Component YAMLs too?

[robertojrojas]

I like state.redis/v1 for ResourceName.
@artursouza any thoughts?

[mukundansundar]

I agree to the naming change.

Additionally Is there a place to add the componentName also?
There might be multiple state.redis/v1 used with different component names for different state stores within a single app. Would that be part of metadata?

[ItalyPaleAle]

Great callout.

Either a different key or state.redis/v1/component-name?

[artursouza]

Based on the gRPC schema for ResourceInfo, we should have:

resource_type: state.redis/v1
resource_name: component-name

[ItalyPaleAle]

Agree

[ItalyPaleAle]

Some keys use camelCase, some use snake_case. We should be consistent

Also see my comment above. I think a single resource_name = state.redis/v1 may be more aligned with how we refer to the comoponents everywhere (note that some components have a dot in the name too, such as state.azure.cosmosdb, and we should include the version too)

[robertojrojas]

@ItalyPaleAle The keys for the JSON payload are auto-generated as part of the Marshaling implemented by the Protobuf stub of the error_details.proto

if you look at the annotations, you'll see the resource_type as the JSON key to generate/read.

ResourceType string `protobuf:"bytes,1,opt,name=resource_type,json=resourceType,proto3" json:"resource_type,omitempty"`

[artursouza]

I made some changes that reflect the latest discussions. Can we merge this? @mukundansundar @ItalyPaleAle

[yaron2]

+1 binding

[artursouza]

+1 binding

[daixiang0]

+1 binding