Skip to content

Commit

Permalink
Map gRPC.Internal to 400 in client metrics interceptor
Browse files Browse the repository at this point in the history
This is done to align with the behavior of the
[ExceptionHandlingInterceptor](https://github.com/cashapp/misk/blob/8cffeac185c4b16bee379acfd112670656588d36/misk/src/main/kotlin/misk/web/exceptions/ExceptionHandlingInterceptor.kt#L176-L188)
which is based on the official doc for gRPC-to-HTTP status mappings
https://grpc.github.io/grpc/core/md_doc_http-grpc-status-mapping.html.
This will resolve inconsistencies between the metrics and actual HTTP
response.

Note: This approach maintains the consistencies for services using Misk
but might introduce discrepancies in metrics when interacting with
external gRPC services (e.g Armeria where the original mapping was
taken).
GitOrigin-RevId: aa2b18b2cc90183bae37b42e5ca7605b21ca6b53
  • Loading branch information
rainecp authored and svc-squareup-copybara committed Oct 7, 2024
1 parent 8cf16be commit 9f957b3
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ class ClientMetricsInterceptor private constructor(
GrpcStatus.OK.code -> HTTP_OK
GrpcStatus.CANCELLED.code -> HTTP_CLIENT_CLOSED_REQUEST
GrpcStatus.UNKNOWN.code,
GrpcStatus.INTERNAL.code,
GrpcStatus.DATA_LOSS.code -> HTTP_INTERNAL_SERVER_ERROR

GrpcStatus.INTERNAL.code, // This aligns with the mapping in ExceptionHandler and https://grpc.github.io/grpc/core/md_doc_http-grpc-status-mapping.html
GrpcStatus.INVALID_ARGUMENT.code,
GrpcStatus.FAILED_PRECONDITION.code,
GrpcStatus.OUT_OF_RANGE.code -> HTTP_BAD_REQUEST
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,13 @@ internal class ClientMetricsInterceptorTest {
assertThat(clientHttp2.ping(AppRequest(200)).execute().code()).isEqualTo(200)

SoftAssertions.assertSoftly { softly ->
softly.assertThat(requestDurationSummary.labels("pingerHttp2.ping", "500").get().count.toInt()).isEqualTo(1)
softly.assertThat(requestDurationSummary.labels("pingerHttp2.ping", "500").get().count.toInt()).isEqualTo(0)
softly.assertThat(requestDurationSummary.labels("pingerHttp2.ping", "200").get().count.toInt()).isEqualTo(1)
softly.assertThat(requestDurationSummary.labels("pingerHttp2.ping", "400").get().count.toInt()).isEqualTo(1)
softly.assertThat(requestDurationSummary.labels("pingerHttp2.ping", "400").get().count.toInt()).isEqualTo(2)

softly.assertThat(requestDurationHistogram.labels("pingerHttp2.ping", "500").get().buckets.last().toInt()).isEqualTo(1)
softly.assertThat(requestDurationHistogram.labels("pingerHttp2.ping", "500").get().buckets.last().toInt()).isEqualTo(0)
softly.assertThat(requestDurationHistogram.labels("pingerHttp2.ping", "200").get().buckets.last().toInt()).isEqualTo(1)
softly.assertThat(requestDurationHistogram.labels("pingerHttp2.ping", "400").get().buckets.last().toInt()).isEqualTo(1)
softly.assertThat(requestDurationHistogram.labels("pingerHttp2.ping", "400").get().buckets.last().toInt()).isEqualTo(2)
}
}

Expand Down

0 comments on commit 9f957b3

Please sign in to comment.