Changed over to a handler for metrics recording

Author: Kixiron

src/web/metrics.rs

@@ -99,40 +99,35 @@ fn duration_to_seconds(d: Duration) -> f64 {
     d.as_secs() as f64 + nanos
 }
 
-#[derive(Debug, Clone)]
-pub struct ResponseRecorder {
-    start_time: Instant,
-    route: Option<String>,
+pub struct RequestRecorder {
+    handler: Box<dyn iron::Handler>,
+    route_name: String,
 }
 
-impl ResponseRecorder {
-    #[inline]
-    pub fn new() -> Self {
+impl RequestRecorder {
+    pub fn new(handler: impl iron::Handler, route: impl Into<String>) -> Self {
         Self {
-            start_time: Instant::now(),
-            route: None,
+            handler: Box::new(handler),
+            route_name: route.into(),
         }
     }
-
-    #[inline]
-    pub fn route(&mut self, route: impl Into<String>) {
-        self.route = Some(route.into());
-    }
 }
 
-impl Drop for ResponseRecorder {
-    fn drop(&mut self) {
-        if let Some(route) = &self.route {
-            ROUTES_VISITED.with_label_values(&[route]).inc();
+impl iron::Handler for RequestRecorder {
+    fn handle(&self, request: &mut Request) -> IronResult<Response> {
+        let start = Instant::now();
+        let result = self.handler.handle(request);
+        let resp_time = duration_to_seconds(start.elapsed());
 
-            let response_time = duration_to_seconds(self.start_time.elapsed());
-            RESPONSE_TIMES
-                .with_label_values(&[route])
-                .observe(response_time);
+        ROUTES_VISITED.with_label_values(&[&self.route_name]).inc();
+        RESPONSE_TIMES
+            .with_label_values(&[&self.route_name])
+            .observe(resp_time);
 
-            #[cfg(test)]
-            tests::record_tests(route);
-        }
+        #[cfg(test)]
+        tests::record_tests(&self.route_name);
+
+        result
     }
 }
 
@@ -178,14 +173,14 @@ mod tests {
             frontend.get("/").send()?;
             frontend.get("/").send()?;
             assert_eq!(ROUTES_VISITED.load(Ordering::SeqCst), 2);
-            assert_eq!(RESPONSE_TIMES.lock().unwrap().get("home (found)"), Some(&2));
+            assert_eq!(RESPONSE_TIMES.lock().unwrap().get("/"), Some(&2));
 
             reset_records();
 
             frontend.get("").send()?;
             frontend.get("").send()?;
             assert_eq!(ROUTES_VISITED.load(Ordering::SeqCst), 2);
-            assert_eq!(RESPONSE_TIMES.lock().unwrap().get("home (found)"), Some(&2));
+            assert_eq!(RESPONSE_TIMES.lock().unwrap().get("/"), Some(&2));
 
             Ok(())
         })
@@ -202,6 +197,7 @@ mod tests {
                 "/menu.js",
                 "/sitemap.xml",
                 "/opensearch.xml",
+                "/robots.txt",
             ];
 
             for route in routes.iter() {
@@ -212,19 +208,11 @@ mod tests {
 
                 assert_eq!(ROUTES_VISITED.load(Ordering::SeqCst), 2);
                 assert_eq!(
-                    RESPONSE_TIMES.lock().unwrap().get("resources (found)"),
+                    RESPONSE_TIMES.lock().unwrap().get("static resource"),
                     Some(&2)
                 );
             }
 
-            reset_records();
-
-            frontend.get("/robots.txt").send()?;
-            frontend.get("/robots.txt").send()?;
-
-            assert_eq!(ROUTES_VISITED.load(Ordering::SeqCst), 2);
-            assert_eq!(RESPONSE_TIMES.lock().unwrap().get("resources"), Some(&2));
-
             Ok(())
         })
     }
@@ -248,19 +236,15 @@ mod tests {
             let frontend = env.frontend();
 
             let routes = [
-                ("/releases", "recent releases (found)"),
-                // ("/releases/recent", "recent releases (found)"),
-                ("/releases/recent/1", "recent releases (found)"),
-                ("/releases/feed", "release feed (found)"),
-                ("/releases/queue", "release queue (found)"),
-                // ("/releases/search", "search releases (found)"),
-                // ("/releases/stars", "release stars (found)"),
-                // ("/releases/stars/1", "release stars (found)"),
-                // ("/releases/activity", "release activity (found)"),
-                // ("/releases/failures", "build failures (found)"),
-                // ("/releases/failures/1", "build failures (found)"),
-                ("/releases/recent-failures", "recent failures (found)"),
-                ("/releases/recent-failures/1", "recent failures (found)"),
+                ("/releases", "/releases"),
+                ("/releases/recent/1", "/releases/recent/:page"),
+                ("/releases/feed", "static resource"),
+                ("/releases/queue", "/releases/queue"),
+                ("/releases/recent-failures", "/releases/recent-failures"),
+                (
+                    "/releases/recent-failures/1",
+                    "/releases/recent-failures/:page",
+                ),
             ];
 
             for (route, correct) in routes.iter() {
@@ -293,29 +277,19 @@ mod tests {
 
             let frontend = env.frontend();
 
-            let routes = [
-                ("/crate/rcc", "crate details (found)"),
-                ("/crate/rcc/", "crate details (found)"),
-                ("/crate/hexponent", "crate details (found)"),
-                ("/crate/hexponent/", "crate details (found)"),
-                ("/crate/rcc/0.0.0", "crate details (found)"),
-                ("/crate/rcc/0.0.0/", "crate details (found)"),
-                ("/crate/hexponent/0.2.0", "crate details (found)"),
-                ("/crate/hexponent/0.2.0/", "crate details (found)"),
-                ("/crate/i_dont_exist", "crate details (404)"),
-                ("/crate/i_dont_exist/", "crate details (404)"),
-                ("/crate/i_dont_exist/4.0.4", "crate details (404)"),
-                ("/crate/i_dont_exist/4.0.4/", "crate details (404)"),
-            ];
+            let routes = ["/crate/rcc/0.0.0", "/crate/hexponent/0.2.0"];
 
-            for (route, correct) in routes.iter() {
+            for route in routes.iter() {
                 reset_records();
 
                 frontend.get(route).send()?;
                 frontend.get(route).send()?;
 
                 assert_eq!(ROUTES_VISITED.load(Ordering::SeqCst), 2);
-                assert_eq!(RESPONSE_TIMES.lock().unwrap().get(*correct), Some(&2));
+                assert_eq!(
+                    RESPONSE_TIMES.lock().unwrap().get("/crate/:name/:version"),
+                    Some(&2)
+                );
             }
 
             Ok(())

src/web/mod.rs

@@ -43,19 +43,6 @@ macro_rules! extension {
     };
 }
 
-/// Formats a route message with the status of the response
-macro_rules! msg {
-    ($resp:expr, $msg:expr) => {{
-        use iron::status::Status;
-
-        match $resp.map_or_else(|err| err.response.status, |resp| resp.status) {
-            Some(Status::Ok) | Some(Status::Found) => format!("{} (found)", $msg),
-            Some(Status::NotFound) => format!("{} (404)", $msg),
-            _ => $msg.to_string(),
-        }
-    }};
-}
-
 mod builds;
 mod crate_details;
 mod error;
@@ -73,11 +60,7 @@ use handlebars_iron::{DirectorySource, HandlebarsEngine};
 use iron::headers::{CacheControl, CacheDirective, ContentType, Expires, HttpDate};
 use iron::modifiers::Redirect;
 use iron::prelude::*;
-use iron::{
-    self,
-    status::{self, Status},
-    Handler, Listening, Url,
-};
+use iron::{self, status, Handler, Listening, Url};
 use postgres::Connection;
 use router::NoRoute;
 use rustc_serialize::json::{Json, ToJson};
@@ -158,12 +141,9 @@ impl CratesfyiHandler {
 
 impl Handler for CratesfyiHandler {
     fn handle(&self, req: &mut Request) -> IronResult<Response> {
-        let mut metrics = metrics::ResponseRecorder::new();
-
         // try serving shared rustdoc resources first, then router, then db/static file handler
         // return 404 if none of them return Ok
-        let response = self
-            .shared_resource_handler
+        self.shared_resource_handler
             .handle(req)
             .or_else(|e| self.router_handler.handle(req).or(Err(e)))
             .or_else(|e| {
@@ -208,80 +188,7 @@ impl Handler for CratesfyiHandler {
                 }
 
                 Self::chain(&self.pool_factory, err).handle(req)
-            });
-
-        let path: &[&str] = &req.url.path();
-        match path {
-            [""] => metrics.route(msg!(response.as_ref(), "home")),
-
-            ["style.css"]
-            | ["index.js"]
-            | ["menu.js"]
-            | ["robots.txt"]
-            | ["sitemap.xml"]
-            | ["opensearch.xml"] => metrics.route(msg!(response.as_ref(), "resources")),
-
-            ["about"] => metrics.route(msg!(response.as_ref(), "about")),
-            ["about", "metrics"] => { /* Metrics route for prometheus */ }
-
-            ["releases"] | ["releases", "recent", _] => {
-                metrics.route(msg!(response.as_ref(), "recent releases"));
-            }
-            ["releases", "feed"] => metrics.route(msg!(response.as_ref(), "release feed")),
-            ["releases", "queue"] => metrics.route(msg!(response.as_ref(), "release queue")),
-            ["releases", "search"] => metrics.route(msg!(response.as_ref(), "search releases")),
-            ["releases", "stars", ..] => metrics.route(msg!(response.as_ref(), "release stars")),
-            ["releases", "activity"] => metrics.route(msg!(response.as_ref(), "release activity")),
-            ["releases", "failures", ..] => {
-                metrics.route(msg!(response.as_ref(), "build failures"));
-            }
-            ["releases", "recent-failures", ..] => {
-                metrics.route(msg!(response.as_ref(), "recent failures"));
-            }
-
-            ["crate", _] | ["crate", _, ""] | ["crate", _, _, ""] => {
-                if let Some(Status::NotFound) = response
-                    .as_ref()
-                    .map_or_else(|err| err.response.status, |resp| resp.status)
-                {
-                    // Crates that don't exist won't be redirected
-                    metrics.route(msg!(response.as_ref(), "crate details"));
-                } else {
-                    // `/crate/:name` and `/crate/:name/` redirect to `/crate/:name/:version`
-                    // `/crate/:name/:version/` redirects to `/crate/:name/:version`
-                }
-            }
-            ["crate", _, _] => {
-                metrics.route(msg!(response.as_ref(), "crate details"));
-            }
-            ["crate", _, _, _, "builds"]
-            | ["crate", _, _, _, "builds.json"]
-            | ["crate", _, _, _, "builds", _] => {
-                metrics.route(msg!(response.as_ref(), "crate builds"));
-            }
-            ["crate", _, _, _, "source", ..] => {
-                metrics.route(msg!(response.as_ref(), "crate source"));
-            }
-            ["crate", _, _, _, "target-redirect", ..] => {
-                metrics.route(msg!(response.as_ref(), "crate target redirect"));
-            }
-
-            [_, "badge.svg"] => metrics.route(msg!(response.as_ref(), "crate badge")),
-            [_, _, "settings.html"] => metrics.route(msg!(response.as_ref(), "crate settings")),
-
-            [_, _] | [_, _, _] => metrics.route(msg!(response.as_ref(), "crate docs")),
-            [_, _, _, file] if file.ends_with(".html") => {
-                metrics.route(msg!(response.as_ref(), "crate docs"));
-            }
-
-            [route] if routes::DOC_RUST_LANG_ORG_REDIRECTS.contains(route) => {
-                metrics.route(msg!(response.as_ref(), "rust-lang redirect"));
-            }
-
-            _ => metrics.route(msg!(response.as_ref(), "crate docs")),
-        }
-
-        response
+            })
     }
 }
 

src/web/routes.rs

@@ -1,3 +1,4 @@
+use super::metrics::RequestRecorder;
 use crate::web::{INDEX_JS, MENU_JS};
 use iron::middleware::Handler;
 use iron::Request;
@@ -184,7 +185,10 @@ impl Routes {
 
     /// A static resource is a normal page without any special behavior on the router side.
     fn static_resource(&mut self, pattern: &str, handler: impl Handler) {
-        self.get.push((pattern.to_string(), Box::new(handler)));
+        self.get.push((
+            pattern.to_string(),
+            Box::new(RequestRecorder::new(handler, "static resource")),
+        ));
     }
 
     /// Internal pages are docs.rs's own pages, instead of the documentation of a crate uploaded by
@@ -197,16 +201,22 @@ impl Routes {
     /// - If the page URL doesn't end with a slash, a redirect from the URL with the trailing slash
     /// to the one without is automatically added.
     fn internal_page(&mut self, pattern: &str, handler: impl Handler) {
-        self.get.push((pattern.to_string(), Box::new(handler)));
+        self.get.push((
+            pattern.to_string(),
+            Box::new(RequestRecorder::new(handler, pattern)),
+        ));
 
         // Automatically add another route ending with / that redirects to the slash-less route.
         if !pattern.ends_with('/') {
             let pattern = format!("{}/", pattern);
             self.get.push((
-                pattern,
-                Box::new(SimpleRedirect::new(|url| {
-                    url.set_path(&url.path().trim_end_matches('/').to_string())
-                })),
+                pattern.to_string(),
+                Box::new(RequestRecorder::new(
+                    SimpleRedirect::new(|url| {
+                        url.set_path(&url.path().trim_end_matches('/').to_string())
+                    }),
+                    pattern,
+                )),
             ));
         }
 
@@ -225,8 +235,10 @@ impl Routes {
     /// resource, but path prefixes are automatically blacklisted (see internal pages to learn more
     /// about page prefixes).
     fn rustdoc_page(&mut self, pattern: &str, handler: impl Handler) {
-        self.rustdoc_get
-            .push((pattern.to_string(), Box::new(handler)));
+        self.get.push((
+            pattern.to_string(),
+            Box::new(RequestRecorder::new(handler, "rustdoc page")),
+        ));
     }
 }