New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Observation instrumentation for gRPC client and server #3427
Conversation
...rc/main/java/io/micrometer/core/instrument/binder/grpc/ObservationGrpcServerInterceptor.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 1 small remark but other than that this looks great!
...rc/main/java/io/micrometer/core/instrument/binder/grpc/ObservationGrpcServerInterceptor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/micrometer/core/instrument/binder/grpc/ObservationGrpcServerInterceptor.java
Show resolved
Hide resolved
|
...rc/main/java/io/micrometer/core/instrument/binder/grpc/ObservationGrpcServerInterceptor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/io/micrometer/core/instrument/binder/grpc/ObservationGrpcClientInterceptor.java
Outdated
Show resolved
Hide resolved
* | ||
* @author Tadaya Tsuyukubo | ||
*/ | ||
public class GrpcObservationSample { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this to micrometer-samples or keep it here?
I'm totally fine keeping it since it is quite simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, keeping it here.
When all sample classes move to micrometer-samples, this goes as well.
* @author Tadaya Tsuyukubo | ||
* @since 1.10.0 | ||
*/ | ||
public enum GrpcObservation implements ObservationDocumentation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since DocumentedObservation
was renamed to ObservationDocumentation
, should we rename GrpcObservation
to to reflect that it is a documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to GrpcObservationDocumentation
to align with existing other documentation classes.
Add Observation instrumentation for gRPC client and server.
This PR adds two gRPC interceptors -
ObservationGrpcClientInterceptor
andObservationGrpcServerInterceptor
.I tried to match the data collecting from
Observation
similar to Sleuth/Brave for spans and existing instrumentation(MetricsCollecting[Client|Server]Interceptor
).For metrics, it is slightly different due to the underlying
Observation
infrastructure andDefaultMeterObservationHandler
.The existing interceptors create these metrics:
grpc.client.requests.sent
(Counter)grpc.client.responses.received
(Counter)grpc.client.processing.duration
(Timer)grpc.server.requests.sent
(Counter)grpc.server.responses.received
(Counter)grpc.server.processing.duration
(Timer)The new
Observation
based interceptors creates metrics:grpc.client.received
(Counter)grpc.client.sent
(Counter)grpc.client
(Timer)grpc.client.active
(LongTaskTimer)grpc.server.received
(Counter)grpc.server.sent
(Counter)grpc.server
(Timer)grpc.server.active
(LongTaskTimer)The spans are nearly equivalent to what Brave provide.
The existing instrumentation does not have a test.
I have added one for the new interceptors that covers all GRPC use cases, 1:1, 1:stream, stream:1, stream:stream with success and failure patterns.