Merge pull request #150 from palantir/fix-metric

Sync client.request metric up to Dialogue

Cargo.toml

@@ -1,3 +1,6 @@
+[workspace.package]
+version = "4.3.0"
+
 [workspace]
 members = [
     "conjure-runtime",

changelog/@unreleased/pr-150.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Updated the `client.response` metric to match Dialogue.
+  links:
+  - https://github.com/palantir/conjure-rust-runtime/pull/150

conjure-runtime-config/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "conjure-runtime-config"
-version = "4.2.0"
+version.workspace = true
 authors = ["Steven Fackler <sfackler@palantir.com>"]
 edition = "2018"
 license = "Apache-2.0"

conjure-runtime/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "conjure-runtime"
-version = "4.2.0"
+version.workspace = true
 authors = ["Steven Fackler <sfackler@palantir.com>"]
 edition = "2018"
 license = "Apache-2.0"
@@ -45,7 +45,7 @@ tower-service = "0.3"
 url = "2.0"
 zipkin = "0.4"
 
-conjure-runtime-config = { version = "4.2.0", path = "../conjure-runtime-config" }
+conjure-runtime-config = { version = "4.3.0", path = "../conjure-runtime-config" }
 
 [dev-dependencies]
 futures-test = "0.3"

conjure-runtime/src/lib.rs

@@ -162,12 +162,11 @@
 //!
 //! ### Standard Metrics
 //!
-//! * `client.request (service: <service_name>)` - A `Timer` recording request durations, tagged by service. Note that
-//!     the requests timed by this metric are the user-percieved request, including any backoffs/retries/etc. It only
-//!     records the time until response headers are received, not until the entire response body is read.
-//! * `client.request.error (service: <service_name>, reason: IOException)` - A `Meter` tracking the rate of IO errors,
-//!     tagged by service. Like the `client.request` metric, this tracks overall user-perceived request. The `reason`
-//!     tag has a value of `IOException` to align with [`dialogue`]'s metric.
+//! * `client.response (channel-name: <channel_name>, service-name: <service_name>, endpoint: <endpoint>, status:
+//!     <status>)` - A `Timer` recording request durations per endpoint. Note that the requests timed by this metric
+//!     are the user-percieved request, including any backoffs/retries/etc. It only records the time until response
+//!     headers are received, not until the entire response body is read. The `status` tag will be `success` if the
+//!     response status was 2xx and will be `failure` otherwise (QoS failure, internal server error, IO error, etc).
 //! * `tls.handshake (context: <service_name>, protocol: <protocol_version>, cipher: <cipher_name>)` - A `Meter`
 //!     tracking the rate of TLS handshakes, tagged by the service, TLS protocol version (e.g. `TLSv1.3`), and cipher
 //!     name (e.g. `TLS_CHACHA20_POLY1305_SHA256`).

conjure-runtime/src/service/metrics.rs

@@ -12,22 +12,23 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 use crate::raw::Service;
-use crate::service::map_error::RawClientError;
 use crate::service::Layer;
 use crate::Builder;
 use conjure_error::Error;
+use conjure_http::client::Endpoint;
 use futures::ready;
+use http::{Request, Response};
 use pin_project::pin_project;
 use std::future::Future;
 use std::pin::Pin;
 use std::sync::Arc;
 use std::task::{Context, Poll};
 use tokio::time::Instant;
-use witchcraft_metrics::{Meter, MetricId, Timer};
+use witchcraft_metrics::{MetricId, MetricRegistry};
 
 struct Metrics {
-    response_timer: Arc<Timer>,
-    io_error_meter: Arc<Meter>,
+    metrics: Arc<MetricRegistry>,
+    service_name: String,
 }
 
 /// A layer which updates the `client.response` and `client.response.error` metrics.
@@ -42,15 +43,8 @@ impl MetricsLayer {
         MetricsLayer {
             metrics: builder.get_metrics().map(|m| {
                 Arc::new(Metrics {
-                    response_timer: m.timer(
-                        MetricId::new("client.response")
-                            .with_tag("service-name", service.to_string()),
-                    ),
-                    io_error_meter: m.meter(
-                        MetricId::new("client.response.error")
-                            .with_tag("service-name", service.to_string())
-                            .with_tag("reason", "IOException"),
-                    ),
+                    metrics: m.clone(),
+                    service_name: service.to_string(),
                 })
             }),
         }
@@ -73,16 +67,21 @@ pub struct MetricsService<S> {
     metrics: Option<Arc<Metrics>>,
 }
 
-impl<S, R> Service<R> for MetricsService<S>
+impl<S, B1, B2> Service<Request<B1>> for MetricsService<S>
 where
-    S: Service<R, Error = Error>,
+    S: Service<Request<B1>, Response = Response<B2>, Error = Error>,
 {
     type Response = S::Response;
     type Error = S::Error;
     type Future = MetricsFuture<S::Future>;
 
-    fn call(&self, req: R) -> Self::Future {
+    fn call(&self, req: Request<B1>) -> Self::Future {
         MetricsFuture {
+            endpoint: req
+                .extensions()
+                .get::<Endpoint>()
+                .expect("Request extensions missing Endpoint")
+                .clone(),
             future: self.inner.call(req),
             start: Instant::now(),
             metrics: self.metrics.clone(),
@@ -95,12 +94,13 @@ pub struct MetricsFuture<F> {
     #[pin]
     future: F,
     start: Instant,
+    endpoint: Endpoint,
     metrics: Option<Arc<Metrics>>,
 }
 
-impl<F, R> Future for MetricsFuture<F>
+impl<F, B> Future for MetricsFuture<F>
 where
-    F: Future<Output = Result<R, Error>>,
+    F: Future<Output = Result<Response<B>, Error>>,
 {
     type Output = F::Output;
 
@@ -110,11 +110,21 @@ where
         let result = ready!(this.future.poll(cx));
 
         if let Some(metrics) = this.metrics {
-            match &result {
-                Ok(_) => metrics.response_timer.update(this.start.elapsed()),
-                Err(e) if e.cause().is::<RawClientError>() => metrics.io_error_meter.mark(1),
-                _ => {}
-            }
+            let status = match &result {
+                Ok(_) => "success",
+                Err(_) => "failure",
+            };
+
+            metrics
+                .metrics
+                .timer(
+                    MetricId::new("client.response")
+                        .with_tag("channel-name", metrics.service_name.clone())
+                        .with_tag("service-name", this.endpoint.service())
+                        .with_tag("endpoint", this.endpoint.name())
+                        .with_tag("status", status),
+                )
+                .update(this.start.elapsed());
         }
 
         Poll::Ready(result)

conjure-verification-api/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "conjure-verification-api"
-version = "2.2.0"
+version.workspace = true
 authors = ["Steven Fackler <sfackler@palantir.com>"]
 edition = "2018"
 publish = false

conjure-verification/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "conjure-verification"
-version = "2.2.0"
+version.workspace = true
 authors = ["Steven Fackler <sfackler@palantir.com>"]
 edition = "2018"
 publish = false

simulation/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "simulation"
-version = "2.2.0"
+version.workspace = true
 authors = ["Steven Fackler <sfackler@gmail.com>"]
 edition = "2018"