feat: observability layer #370
Conversation
| @@ -129,60 +125,6 @@ pub fn set_num_subnet_nodes(nodes: u32) { | |||
| }); | |||
| } | |||
|
|
|||
| pub fn http_client( | |||
There was a problem hiding this comment.
moved this part to http.rs
| ) -> impl Service<CanisterHttpRequestArgument, Response = HttpResponse, Error = RpcError> { | ||
| ServiceBuilder::new() | ||
| .layer( | ||
| ObservabilityLayer::new() |
There was a problem hiding this comment.
this is the main starting point.
There was a problem hiding this comment.
Thanks for the PR @gregorydemay! A few nits regarding mostly the documentation, but otherwise looks good! Had to wrap my head a bit around the number of generics but the end result is quite cool.
| impl<OnRequest, OnResponse, OnError> ObservabilityLayer<OnRequest, OnResponse, OnError> { | ||
| /// Customize what to do when a request is received. | ||
| /// | ||
| /// `NewOnRequest` is expected to implement [`RequestObserver`]. |
There was a problem hiding this comment.
Understanding question: why do we not require NewOnRequest to implement RequestObserver here? What would be a use-case where NewOnRequest does not implement RequestObserver?
| type Service = Observability<S, OnRequest, OnResponse, OnError>; | ||
|
|
||
| fn layer(&self, inner: S) -> Self::Service { | ||
| Self::Service { |
There was a problem hiding this comment.
nit: I feel like using Observability here is clearer as this is what we are actually initializing.
| Self::Service { | |
| Observability { |
|
|
||
| /// Middleware that adds high level observability to a [`Service`]. | ||
| /// | ||
| /// See the [module docs](crate::observability) for an example. |
There was a problem hiding this comment.
nit: the example here is missing.
| impl<OnRequest, OnResponse, OnError> ObservabilityLayer<OnRequest, OnResponse, OnError> { | ||
| /// Customize what to do when a request is received. | ||
| /// | ||
| /// `NewOnRequest` is expected to implement [`RequestObserver`]. |
There was a problem hiding this comment.
nit: it seems the link to RequestObserver in the doc is broken here
It looks exposing the type in lib.rs fixes the problem.
| @@ -9,6 +9,8 @@ pub use client::{Client, IcError}; | |||
| pub use cycles::{ | |||
| CyclesAccounting, CyclesAccountingError, CyclesChargingPolicy, CyclesCostEstimator, | |||
| }; | |||
| pub use observability::ObservabilityLayer; | |||
There was a problem hiding this comment.
nit: this fixes a few doc warnings:
| pub use observability::ObservabilityLayer; | |
| pub use observability::{Observability, ObservabilityLayer, RequestObserver, ResponseObserver}; |
|
|
||
| /// Trait used to tell [`Observability`] what to do when a response is received. | ||
| pub trait ResponseObserver<RequestData, Result> { | ||
| /// Observe the response (typically an instance of [`std::Result`] and the request data produced by a [`RequestObserver`]. |
There was a problem hiding this comment.
nit: unmatched bracket
| /// Observe the response (typically an instance of [`std::Result`] and the request data produced by a [`RequestObserver`]. | |
| /// Observe the response (typically an instance of [`std::Result`]) and the request data produced by a [`RequestObserver`]. |
| } | ||
|
|
||
| /// Trait used to tell [`Observability`] what to do when a response is received. | ||
| pub trait ResponseObserver<RequestData, Result> { |
There was a problem hiding this comment.
nit: I feel like this should be Response. ResponseObserver would then more closely mirror RequestObserver, and I feel it's more telling of what this actually is than just Request. I know that this is not always an HTTP response, and can be e.g. an error, but I feel like this is a type of response. WDYT?
Don't want to bikeshed the naming of generic arguments, but given that there's quite a handful here I do think it makes sense to try to make everything as readable as possible.
| } | ||
| } | ||
|
|
||
| impl<F, ReqData, T> ResponseObserver<ReqData, T> for F |
There was a problem hiding this comment.
nit: I think an example with a closure here could be pretty useful. Could also potentially be in the top-level docs.
Follow-up on #364 to add a new observability layer to take care of logging and metrics. Unfortunately,
tower_http::tracecannot be used in a canister environment because it measures call latency withInstant::now. The proposed layer, while inspired bytower_http::trace, is also simpler because it does not have to deal with streaming responses.