[PR Feedback] Only send execution duration if health check hook exists

Signed-off-by: Christopher Maier <[email protected]>

components/sup/protocols/event.proto

@@ -76,8 +76,7 @@ message HealthCheckEvent {
   ServiceMetadata service_metadata = 2;
   // The result of the health check execution
   HealthCheck result = 3;
-  // How long the health check took to run
+  // If the service has a health check hook script, how long it took
+  // to execute.
   google.protobuf.Duration duration = 4;
-  // Whether or not the health check is defined in a hook
-  bool has_hook = 5;
 }

components/sup/src/event.rs

@@ -182,17 +182,13 @@ pub fn service_stopped(service: &Service) {
     }
 }
 
-pub fn health_check(service: &Service,
-                    check_result: HealthCheck,
-                    duration: Duration,
-                    has_hook: bool) {
+pub fn health_check(service: &Service, check_result: HealthCheck, duration: Option<Duration>) {
     if stream_initialized() {
         let check_result: types::HealthCheck = check_result.into();
         publish(HealthCheckEvent { service_metadata: Some(service.to_service_metadata()),
-                                   event_metadata: None,
-                                   result: i32::from(check_result),
-                                   duration: Some(duration.into()),
-                                   has_hook });
+                                   event_metadata:   None,
+                                   result:           i32::from(check_result),
+                                   duration:         duration.map(Into::into), });
     }
 }
 

components/sup/src/manager/service/mod.rs

@@ -94,10 +94,8 @@ use std::{self,
           result,
           sync::{Arc,
                  RwLock},
-          time::{Duration,
-                 Instant}};
-use time::{SteadyTime,
-           Timespec};
+          time::Instant};
+use time::Timespec;
 
 static LOGKEY: &'static str = "SR";
 
@@ -934,19 +932,25 @@ impl Service {
         // for Prometheus (see `_timer`, above), but we can't inspect
         // that and get the elapsed time. Thus, we keep track of time
         // ourselves.
-        let event_start = SteadyTime::now();
+        //
+        // Additionally, we'll only send the time with the health
+        // check event if there's actually a hook that runs. We'll
+        // default to there not being a hook, though.
+        let mut event_duration = None;
+
         let check_result = if let Some(ref hook) = self.hooks.health_check {
-            hook.run(&self.service_group,
-                     &self.pkg,
-                     self.svc_encrypted_password.as_ref())
+            let event_start = Instant::now();
+            let result = hook.run(&self.service_group,
+                                  &self.pkg,
+                                  self.svc_encrypted_password.as_ref());
+            event_duration = Some(event_start.elapsed());
+            result
         } else {
             match self.supervisor.status() {
                 (true, _) => HealthCheck::Ok,
                 (false, _) => HealthCheck::Critical,
             }
         };
-        let event_stop = SteadyTime::now();
-        let event_duration = (event_stop - event_start).to_std().unwrap_or_default();
 
         // We have just ran a check; therefore we must unset the next scheduled check time
         // in anticipation of `None` value being used in the next scheduled check time calculation.
@@ -962,12 +966,7 @@ impl Service {
             self.schedule_special_health_check();
         }
         self.health_check = check_result;
-        event::health_check(&self,
-                            check_result,
-                            event_duration,
-                            // TODO (CM): self.hooks is not public,
-                            // and it's not clear that it should be.
-                            self.hooks.health_check.is_some());
+        event::health_check(&self, check_result, event_duration);
         self.cache_health_check(check_result);
     }