mononoke: Add a LfsMethod enum to allow per-method histogram configs

Summary:
Previously, the RequestContext stored a string representing the method
type. Replace this with an enum, so that matching on it is easier - allowing
for different histogram configs for each method type.

For the batch endpoint, use a histogram of durations from 0 -> 500ms.

Reviewed By: farnz

Differential Revision: D17530495

fbshipit-source-id: 23ff424fc0aca5c1022f9d6521bbd70d0af3ccf6

lfs_server/src/batch.rs

@@ -24,14 +24,12 @@ use mononoke_types::{hash::Sha256, typed_hash::ContentId, MononokeId};
 use crate::errors::ErrorKind;
 use crate::http::{git_lfs_mime, BytesBody, HttpError, TryIntoResponse};
 use crate::lfs_server_context::{RepositoryRequestContext, UriBuilder};
-use crate::middleware::ScubaMiddlewareState;
+use crate::middleware::{LfsMethod, ScubaMiddlewareState};
 use crate::protocol::{
     ObjectAction, ObjectError, ObjectStatus, Operation, RequestBatch, RequestObject, ResponseBatch,
     ResponseObject, Transfer,
 };
 
-const METHOD: &str = "batch";
-
 define_stats! {
     prefix ="mononoke.lfs.batch";
     download_redirect_internal: timeseries(RATE, SUM),
@@ -369,7 +367,7 @@ async fn batch_download(
 pub async fn batch(state: &mut State) -> Result<impl TryIntoResponse, HttpError> {
     let BatchParams { repository } = state.take();
 
-    let ctx = RepositoryRequestContext::instantiate(state, repository.clone(), METHOD)
+    let ctx = RepositoryRequestContext::instantiate(state, repository.clone(), LfsMethod::Batch)
         .map_err(HttpError::e400)?;
 
     let body = Body::take_from(state)

lfs_server/src/download.rs

@@ -17,8 +17,7 @@ use stats::{define_stats, Histogram};
 use crate::errors::ErrorKind;
 use crate::http::{HttpError, StreamBody, TryIntoResponse};
 use crate::lfs_server_context::RepositoryRequestContext;
-
-const METHOD: &str = "download";
+use crate::middleware::LfsMethod;
 
 define_stats! {
     prefix ="mononoke.lfs.download";
@@ -37,7 +36,7 @@ pub async fn download(state: &mut State) -> Result<impl TryIntoResponse, HttpErr
         content_id,
     } = state.take();
 
-    let ctx = RepositoryRequestContext::instantiate(state, repository.clone(), METHOD)
+    let ctx = RepositoryRequestContext::instantiate(state, repository.clone(), LfsMethod::Download)
         .map_err(HttpError::e400)?;
 
     let content_id = ContentId::from_str(&content_id)

lfs_server/src/lfs_server_context.rs

@@ -30,7 +30,7 @@ use hyper_openssl::HttpsConnector;
 use mononoke_types::ContentId;
 
 use crate::errors::ErrorKind;
-use crate::middleware::RequestContext;
+use crate::middleware::{LfsMethod, RequestContext};
 use crate::protocol::{RequestBatch, RequestObject, ResponseBatch};
 
 pub type HttpsHyperClient = Client<HttpsConnector<HttpConnector>>;
@@ -112,7 +112,7 @@ impl RepositoryRequestContext {
     pub fn instantiate(
         state: &mut State,
         repository: String,
-        method: &'static str,
+        method: LfsMethod,
     ) -> Result<Self, Error> {
         if let Some(ctx) = state.try_borrow_mut::<RequestContext>() {
             ctx.set_request(repository.clone(), method);

lfs_server/src/middleware/mod.rs

@@ -18,7 +18,7 @@ mod timer;
 pub use self::identity::IdentityMiddleware;
 pub use self::log::LogMiddleware;
 pub use self::ods::OdsMiddleware;
-pub use self::request_context::{RequestContext, RequestContextMiddleware};
+pub use self::request_context::{LfsMethod, RequestContext, RequestContextMiddleware};
 pub use self::scuba::{ScubaMiddleware, ScubaMiddlewareState};
 pub use self::timer::TimerMiddleware;
 

lfs_server/src/middleware/ods.rs

@@ -10,7 +10,7 @@ use hyper::{Body, Response};
 use stats::{define_stats, DynamicHistogram, DynamicTimeseries};
 use time_ext::DurationExt;
 
-use super::{Middleware, RequestContext};
+use super::{LfsMethod, Middleware, RequestContext};
 
 define_stats! {
     prefix = "mononoke.lfs.request";
@@ -18,18 +18,25 @@ define_stats! {
     success: dynamic_timeseries("{}.success", (repo_and_method: String); RATE, SUM),
     failure_4xx: dynamic_timeseries("{}.failure_4xx", (repo_and_method: String); RATE, SUM),
     failure_5xx: dynamic_timeseries("{}.failure_5xx", (repo_and_method: String); RATE, SUM),
-    duration: dynamic_histogram("{}_ms", (repo_and_method: String); 100, 0, 5000, AVG, SUM, COUNT; P 5; P 25; P 50; P 75; P 95; P 97; P 99),
+    upload_duration: dynamic_histogram("{}.upload_ms", (repo: String); 100, 0, 5000, AVG, SUM, COUNT; P 5; P 25; P 50; P 75; P 95; P 97; P 99),
+    download_duration: dynamic_histogram("{}.download_ms", (repo: String); 100, 0, 5000, AVG, SUM, COUNT; P 5; P 25; P 50; P 75; P 95; P 97; P 99),
+    batch_duration: dynamic_histogram("{}.batch_ms", (repo: String); 10, 0, 500, AVG, SUM, COUNT; P 5; P 25; P 50; P 75; P 95; P 97; P 99),
 }
 
 fn log_stats(state: &mut State, status: StatusCode) -> Option<()> {
     let ctx = state.try_borrow_mut::<RequestContext>()?;
-    let repo_and_method = format!("{}.{}", ctx.repository.as_ref()?, ctx.method?);
+    let method = ctx.method?;
+    let repo_and_method = format!("{}.{}", ctx.repository.as_ref()?, method.to_string());
 
     ctx.add_post_request(move |duration| {
-        STATS::duration.add_value(
-            duration.as_millis_unchecked() as i64,
-            (repo_and_method.clone(),),
-        );
+        match method {
+            LfsMethod::Upload => STATS::upload_duration
+                .add_value(duration.as_millis_unchecked() as i64, (method.to_string(),)),
+            LfsMethod::Download => STATS::download_duration
+                .add_value(duration.as_millis_unchecked() as i64, (method.to_string(),)),
+            LfsMethod::Batch => STATS::batch_duration
+                .add_value(duration.as_millis_unchecked() as i64, (method.to_string(),)),
+        }
 
         STATS::requests.add_value(1, (repo_and_method.clone(),));
 

lfs_server/src/middleware/request_context.rs

@@ -4,6 +4,8 @@
 // This software may be used and distributed according to the terms of the
 // GNU General Public License version 2 or any later version.
 
+use std::fmt;
+
 use futures::{Future, IntoFuture};
 use gotham::state::State;
 use gotham_derive::StateData;
@@ -18,10 +20,28 @@ use super::Middleware;
 
 type PostRequestCallback = Box<dyn FnOnce(&Duration) + Sync + Send + 'static>;
 
+#[derive(Copy, Clone)]
+pub enum LfsMethod {
+    Upload,
+    Download,
+    Batch,
+}
+
+impl fmt::Display for LfsMethod {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let name = match self {
+            Self::Upload => "upload",
+            Self::Download => "download",
+            Self::Batch => "batch",
+        };
+        write!(f, "{}", name)
+    }
+}
+
 #[derive(StateData)]
 pub struct RequestContext {
     pub repository: Option<String>,
-    pub method: Option<&'static str>,
+    pub method: Option<LfsMethod>,
     pub error_msg: Option<String>,
     pub response_size: Option<u64>,
     pub headers_duration: Option<Duration>,
@@ -45,7 +65,7 @@ impl RequestContext {
         }
     }
 
-    pub fn set_request(&mut self, repository: String, method: &'static str) {
+    pub fn set_request(&mut self, repository: String, method: LfsMethod) {
         self.repository = Some(repository);
         self.method = Some(method);
     }

lfs_server/src/middleware/scuba.rs

@@ -88,7 +88,7 @@ fn log_stats(state: &mut State, status_code: &StatusCode) -> Option<()> {
     }
 
     if let Some(method) = ctx.method {
-        scuba.add("method", method);
+        scuba.add("method", method.to_string());
     }
 
     if let Some(err_msg) = &ctx.error_msg {

lfs_server/src/upload.rs

@@ -28,7 +28,7 @@ use mononoke_types::hash::Sha256;
 use crate::errors::ErrorKind;
 use crate::http::{EmptyBody, HttpError, TryIntoResponse};
 use crate::lfs_server_context::RepositoryRequestContext;
-use crate::middleware::ScubaMiddlewareState;
+use crate::middleware::{LfsMethod, ScubaMiddlewareState};
 use crate::protocol::{
     ObjectAction, ObjectStatus, Operation, RequestBatch, RequestObject, ResponseBatch, Transfer,
 };
@@ -44,7 +44,6 @@ define_stats! {
 
 // Small buffers for Filestore & Dewey
 const BUFFER_SIZE: usize = 5;
-const METHOD: &str = "upload";
 
 // NOTE: We don't deserialize things beyond a String form, in order to report errors in our
 // controller, not in routing.
@@ -143,7 +142,7 @@ pub async fn upload(state: &mut State) -> Result<impl TryIntoResponse, HttpError
         size,
     } = state.take();
 
-    let ctx = RepositoryRequestContext::instantiate(state, repository.clone(), METHOD)
+    let ctx = RepositoryRequestContext::instantiate(state, repository.clone(), LfsMethod::Upload)
         .map_err(HttpError::e400)?;
 
     let oid = Sha256::from_str(&oid).map_err(HttpError::e400)?;