core: exclude client and hedging cancellations from callcounters for outlier-detection

[AgraVator]

fixes #12834

[kannanjgithub]

This method is for transport callback for stream closure and it is too late at this point to make a distinction whether the cancellation is app/retriable stream initiated or from the remote endpoint. This should instead be done in method cancel that sets this.cancelled = true; and that call only happens for app/retriable stream initiated cancellation, and not for remote server initiated cancellations, and happens before the transport stream is closed.

[kannanjgithub]

In addition, we would need the guard in AbstractClientStream.cancel against late cancellations:

     @Override
     public final void cancel(Status reason) {
       Preconditions.checkArgument(!reason.isOk(), "Should not cancel with OK status");
       
       if (cancelled || transportState().listenerClosed) {   //  <----- GUARD 
         return;
       }
       
      cancelled = true;
      statsTraceCtx.clientCancelled(reason);
      abstractClientStreamSink().cancel(reason);
    }

[AgraVator]

Placing statsTraceCtx.clientCancelled(reason) inside cancel() on the calling thread (app thread or deadline timer thread) could result in a race condition. Had it like that in the initial commits but then eric suggested to use transportReadyStatus() instead.

[kannanjgithub]

Using stopDelivery == true as the only indicator of a client-initiated cancellation is flawed because its meaning is context-dependent:

1. Client Cancels (App / Hedging): Passes stopDelivery = true.
2. Client Timeouts (DEADLINE_EXCEEDED): Passes stopDelivery = true.
3. Transport Forceful Shutdown (UNAVAILABLE): Passes stopDelivery = true.
4. Abrupt Network Drop (UNAVAILABLE): Passes stopDelivery = false.
5. Server Sends Reset (CANCEL or INTERNAL): Passes stopDelivery = false.

(3) is a problem. In Netty transport for example, NettyClientHandler.forcefulClose explicitly sets stopDelivery = true (passing Status.UNAVAILABLE or similar).

Change this to:

         // We must ensure the status code actually reflects a client-initiated action!
         if (stopDelivery && (status.getCode() == Status.Code.CANCELLED || 
                             status.getCode() == Status.Code.DEADLINE_EXCEEDED)) {
          statsTraceCtx.clientCancelled(status);
          }

[ejona86]

No, we definitely shouldn't be doing any logic here based on the status code.

What is wrong with "Transport Forceful Shutdown'? That is defined as cancelling all streams, so it should behave the same as call.cancel().

I can believe we may need to change some cases to stop using stopDelivery=true; I bet there are/were some cases where it previously didn't matter what the value was. I think we can rename stopDelivery to cancelled, as that's the only time we should be discarding data that we've received.

[kannanjgithub]

I wanted to count transport forceful shutdown in response to network drop as failure because it is not client app initiated although it might be client network stack initiated. Envoy does count connection drops as failures (LocalOriginConnectFailed) without any HTTP status code received from the remote peer.

[ejona86]

No, we definitely shouldn't be doing any logic here based on the status code.

What is wrong with "Transport Forceful Shutdown'? That is defined as cancelling all streams, so it should behave the same as call.cancel().

I can believe we may need to change some cases to stop using stopDelivery=true; I bet there are/were some cases where it previously didn't matter what the value was. I think we can rename stopDelivery to cancelled, as that's the only time we should be discarding data that we've received.