Implement Result Code as required by OTLP #47
Conversation
tigrannajaryan
requested review from
AloisReitbauer,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
SergeyKanzhelev,
songy23,
tedsuo and
yurishkuro
as code owners
tigrannajaryan
force-pushed
the
feature/tigran/resultcode
branch
from
5b60ff4 to
ac96f4d
Compare
tigrannajaryan
force-pushed
the
feature/tigran/resultcode
branch
from
ac96f4d to
0c4b764
Compare
This change implements OTLP requirement of Result Code. OTLP describes how it is used in the export responses: https://github.com/open-telemetry/oteps/blob/master/text/0035-opentelemetry-protocol.md#result-code
tigrannajaryan
force-pushed
the
feature/tigran/resultcode
branch
from
0c4b764 to
f2f54b6
Compare
| @@ -95,3 +95,20 @@ message ServiceInfo { | |||
|
|
|||
| // TODO(songya): add more fields as needed. | |||
| } | |||
|
|
|||
| // ExportResultCode is the result code returned in the export responses. | |||
| enum ExportResultCode { | |||
There was a problem hiding this comment.
I remember you told me that this is controlled by the status we return from grpc not by the message.
There was a problem hiding this comment.
grpc status is used for throttling purposes: https://github.com/open-telemetry/oteps/blob/master/text/0035-opentelemetry-protocol.md#throttling
ResultCode is different and is an explicit field in the response.
The reason I am using grpc code for throttling is that there is already clearly defined grpc code for this (Unavailable). As opposed to that there is no grpc code that allows to distinguish between FailedRetryable and FailedNotRetryable cases and I don't want to artificially model them using other grpc codes.
There was a problem hiding this comment.
But from the Status you can determine if a retry is possible or not correct?
There was a problem hiding this comment.
Is there a clearly defined condition / set of status codes that allow client to make a decision about retrying or not retrying based just the grpc status code? If there is such a way then we can eliminate ResultCode.
At the moment I am not sure how a client should make such decision by looking at the returned gRPC code. For example if gRPC Status is Unknown is it retryable or no?
Perhaps instead of ResultCode we list here the status codes which must be interpreted by the client as Retryable and then make a recommendation for server to return specific errors codes to Retryable and NonRetryable cases? That could work.
There was a problem hiding this comment.
FYI: So you always get a status, but I think you never get a response + an error status (if I remember correctly).
So the logic I would use retry if:
- If the status contains the RetryInfo is a retryable
- For some status codes without a RetryInfo (deadline exceeded, need to determine the full list) you may do retry.
There was a problem hiding this comment.
FYI: So you always get a status, but I think you never get a response + an error status (if I remember correctly).
Yes, that is correct. A response only comes when status is success. Any other status is received without a response.
If the status contains the RetryInfo is a retryable
So you are saying RetryInfo with RetryDelay=0 would be equivalent to ResultCode=FailedRetryable. The same RetryInfo with RetryDelay>0 would be throttling indication (also retryable obviously, but after a delay).
For some status codes without a RetryInfo (deadline exceeded, need to determine the full list) you may do retry.
Make sense. Also, we need to decide which status code to return to indicate NotRetryable. Let me think though and submit an update.
There was a problem hiding this comment.
I created open-telemetry/oteps#65
If it gets accepted I will discard this PR.
OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes.
OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes.
OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes.
|
Reviewers, please have a look at open-telemetry/oteps#65 instead. This uses a different approach that Bogdan proposed and which I think is better. |
OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes.
I'm assuming this have to be closed? I'm closing, please re-open if needed |
OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes.
OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes.
OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes.
) OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes. Co-authored-by: Yuri Shkuro <[email protected]>
…pen-telemetry#65) OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes. Co-authored-by: Yuri Shkuro <[email protected]>
…pen-telemetry#65) OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes. Co-authored-by: Yuri Shkuro <[email protected]>
…pen-telemetry#65) OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes. Co-authored-by: Yuri Shkuro <[email protected]>
…pen-telemetry#65) OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes. Co-authored-by: Yuri Shkuro <[email protected]>
…pen-telemetry/oteps#65) OTLP proposal originally used a separate ResultCode enumeration for server to tell the client whether the failed request can be retried or no. After discussion here open-telemetry/opentelemetry-proto#47 (comment) it became clear that the goal can be achieved using gRPC status codes without a need for custom enumeration. This change removes the ResultCode and explains how to use gRPC status codes. Co-authored-by: Yuri Shkuro <[email protected]>
This change implements OTLP requirement of Result Code. OTLP describes
how it is used in the export responses:
https://github.com/open-telemetry/oteps/blob/master/text/0035-opentelemetry-protocol.md#result-code