plumb otel subchannel metrics disconnect error #12342
Conversation
| @GuardedBy("this") | ||
| void notifyShutdown(Status status) { | ||
| clientTransportListener.transportShutdown(status); | ||
| clientTransportListener.transportShutdown(status, SimpleDisconnectError.SUBCHANNEL_SHUTDOWN); |
There was a problem hiding this comment.
It definitely isn't SUBCHANNEL_SHUTDOWN. I think it is fair to use UNKNOWN, because the transport would need to be instrumented to know better.
| @GuardedBy("this") | ||
| void notifyShutdown(Status status) { | ||
| clientTransportListener.transportShutdown(status); | ||
| clientTransportListener.transportShutdown(status, SimpleDisconnectError.SUBCHANNEL_SHUTDOWN); |
There was a problem hiding this comment.
It definitely isn't SUBCHANNEL_SHUTDOWN. I think it is fair to use UNKNOWN, because the transport would need to be instrumented to know better.
| * | ||
| */ | ||
| @ThreadSafe | ||
| public interface ClientTransportWithDisconnectReason extends ClientTransport { |
There was a problem hiding this comment.
This is only used as part of the KeepAliveManager API, so move it there. And then it would be named relative to that API, like Transport if within ClientKeepAlivePinger.
(This is probably the only remaining use of ping() as well, so we'll probably end up moving ping() from ClientTransport to this interface as well, as which point it won't extend ClientTransport.)
There was a problem hiding this comment.
OkHttpClientTransport and NettyClientTransport will implement it as well....
There was a problem hiding this comment.
Because they are users of the ClientKeepAlivePinger API...
|
|
||
| @Override | ||
| public void shutdownNow(final Status reason) { | ||
| shutdownNow(reason, SimpleDisconnectError.UNKNOWN); |
| @Override | ||
| public void run() { | ||
| lifecycleManager.notifyShutdown(reason); | ||
| lifecycleManager.notifyShutdown(reason, SimpleDisconnectError.SUBCHANNEL_SHUTDOWN); |
|
|
||
| @Override | ||
| public void shutdownNow(Status reason) { | ||
| shutdownNow(reason, SimpleDisconnectError.UNKNOWN); |
|
|
||
| @Override | ||
| public void shutdownNow(Status reason, DisconnectError disconnectError) { | ||
| shutdown(reason); |
There was a problem hiding this comment.
disconnectError looks like it should be passed to be passed to shutdown.
| ArgumentCaptor<Status> captor = ArgumentCaptor.forClass(Status.class); | ||
| verify(listener, timeout(TIME_OUT_MS)).transportShutdown(captor.capture()); | ||
| verify(listener, timeout(TIME_OUT_MS)).transportShutdown(captor.capture(), | ||
| any(DisconnectError.class)); |
There was a problem hiding this comment.
Probably want to consider verifying the DisconnectError as well, for this and the other three below that use captor.capture().
Finishes the remaining work of A94