Set the error code based on the HTTP status code

[rakyll]

plugin/ochttp need to set the error code based on the HTTP response code.

Below, there is a reference how HTTP status codes maps to error codes:

	case 200:
		return int32(code.Code_OK)
	case 499:
		return int32(code.Code_CANCELLED)
	case 500:
		return int32(code.Code_UNKNOWN) // Could also be Code_INTERNAL, Code_DATA_LOSS
	case 400:
		return int32(code.Code_INVALID_ARGUMENT) // Could also be Code_OUT_OF_RANGE
	case 504:
		return int32(code.Code_DEADLINE_EXCEEDED)
	case 404:
		return int32(code.Code_NOT_FOUND)
	case 409:
		return int32(code.Code_ALREADY_EXISTS) // Could also be Code_ABORTED
	case 403:
		return int32(code.Code_PERMISSION_DENIED)
	case 401:
		return int32(code.Code_UNAUTHENTICATED)
	case 429:
		return int32(code.Code_RESOURCE_EXHAUSTED)
	case 501:
		return int32(code.Code_UNIMPLEMENTED)
	case 503:
		return int32(code.Code_UNAVAILABLE)
	default:
		return int32(code.Code_UNKNOWN)

[semistrict]

@rakyll where did this code come from?

[bogdandrutu]

https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto

[codefromthecrypt]

what about 418 I'm a Teapot?

[rakyll]

@jadekler suggested that he can add a function to the ochttp package.

HTTPStatusToErrorCode(statusCode int) int32

@jadekler, can I assign it to you?

[jeanbza]

@rakyll Yes, please do

[jba]

trace.Status.Code is an int32, which is a good, agnostic choice. I'm worried that using the Google API/gRPC codes (they are the same) is favoring Google's monitoring stack. Also, converting the HTTP codes to the (far fewer) gRPC codes loses information.

I'd like to understand the full cross product of behavior here. How does Stackdriver/prometheus/etc. handle gRPC codes/HTTP codes/arbitrary int32s?

I think it would be fine if Google's HTTP-based clients used the gRPC codes; it seems reasonable that Google's clients are optimized for Google's monitoring. But none of the code in this repo should have that bias.

So maybe this repo contains HTTPStatusToGRPCCode but does not call it by default.

[semistrict]

I think we need to define what the status code maps to: either HTTP status codes or gRPC. Otherwise it's impossible to interpret in any meaningful way without knowing exactly where a Span was produced, which an exporter will in general not know.

At minimum we need to know that an error occurred (non-successful status) and probably also whether it's a client problem or server problem.

cc @bogdandrutu

[rakyll]

Also, converting the HTTP codes to the (far fewer) gRPC codes loses information.

We are also recording the HTTP status code as an attribute, so no information loss here.

I'd like to understand the full cross product of behavior here. How does Stackdriver/prometheus/etc. handle gRPC codes/HTTP codes/arbitrary int32s

For metrics, they will be labels. For traces, they will be span attributes. Some backends already have first-class canonical label or attribute keys for errors. If that's the case, the exporters should populate them.

HTTPStatusToGRPCCode

It shouldn't be called after gRPC, HTTPStatusToCode is more preferable. This is not a gRPC-specific mapping, we just happened to have the same mapping. That's why it is not Google-specific.

It would be better to argue the rationale behind error codes all together and change the proto if we don't think they are useful.

[jba]

This is not a gRPC-specific mapping, we just happened to have the same mapping. That's why it is not Google-specific.

In that case, you should have your own Code type that is a copy of the gRPC one so you don't have a dependency on gRPC, and you should change the type of trace.Status.Code to it.

[jba]

It would be better to argue the rationale behind error codes all together and change the proto if we don't think they are useful.

Along those lines: if the HTTP code is going to be an attribute anyway, then maybe there shouldn't be a privileged Status.Code field. Instead, provide documented attribute names for each kind of code, and exporters can decide what to do with them. Maybe the stackdriver exporter converts the "http_code" attribute to the gRPC code space, and passes through the "grpc_code" attribute. Another exporter might want to do the opposite.

[rakyll]

In that case, you should have your own Code type that is a copy of the gRPC one so you don't have a dependency on gRPC

We don't have a type dependency on gRPC. What exactly are you pointing at? See https://godoc.org/go.opencensus.io/trace#Status. We will remove the comment there and replace it with our spec for error codes as soon as we have a consensus on this.

Maybe the stackdriver exporter converts the "http_code" attribute to the gRPC code space, and passes through the "grpc_code" attribute. Another exporter might want to do the opposite.

There is no maybe in this. We specify what the exporter behavior should be.
https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/HTTPAttributes.md

I don't understand why you are dealing with any of the HTTP attributes and annotations unless you want to add custom ones.

You should just use the ochttp package and it will provide a basic set of attributes, annotations, etc. We currently set the attributes (example), and will set the error as soon as there is consensus on the mapping. You shouldn't care about them.

[jba]

We don't have a type dependency on gRPC. What exactly are you pointing at?

Sorry for not being clear. Here is what I mean:

If you make the choice to use this set of error codes, then

1. you should change the type of Status.Code from int32 to some other type, like Code, that is an enum.
2. That Code type should not be google.golang.org/grpc/codes.Code, because you don't want to take a dependency on gRPC. Instead, it should be your own copy.

I don't understand why you are dealing with any of the HTTP attributes and annotations...

I'm not. Sorry again about lack of clarity.

In that comment, I was suggesting that you delete the Status.Code field entirely—punt on the decision to pick the One True Set of error codes. Instead:

• ochttp sets the http.status_code attribute
• ocgrpc sets the grpc.status_code attribute
• similarly for any other protocol with its own set of codes

When an exporter's ExportSpan method is called, the exporter looks at SpanData.Attributes and does whatever it wants. For example, the stackdriver exporter's logic might say: "If the grpc.status_code attribute is present, use its value directly in tracepb.Span.Status.Code. Otherwise, if the http.status_code attribute is present, convert it with HTTPStatusToCode and use that. "

But another exporter could make a different choice, choosing to transmit HTTP codes to its backend and converting the gRPC codes in the other direction.

[jeanbza]

Agreed with the above. I'm concerned about,

We don't have a type dependency on gRPC. What exactly are you pointing at? See https://godoc.org/go.opencensus.io/trace#Status. We will remove the comment there and replace it with our spec for error codes as soon as we have a consensus on this.

The intention behind https://godoc.org/go.opencensus.io/trace#Status seems to be to represent gRPC codes. I think the way that this ends up if we don't explicitly break out http and grpc is copy-pasting gRPC's codes into something called opencensus.Code (or, whatever the package) and sort of re-implementing gRPC's documentation and so on.

I'm not convinced that opencensus should have its own codes - I reckon it should be a transparent middleman that facilitates stats/tracing from my app to my exporter, not something I need to "conform" to.

+1 to deletion of Status.Code and instead setting http.status_code and grpc.status_code, and so on.