formalize the timestamp_provider mechanism

- initially I thought that the Config object should be passed around (which I'm not yet convinced that this shouldnt be the case), but making an arbitrary choice here to limit the api surface - only expose the timestamp_provider methods from emitters - open to refactoring later, this is not public crate api Signed-off-by: mimir-d <[email protected]>
opencomputeproject · Oct 8, 2024 · 70167d9 · 70167d9
1 parent cadbe21
commit 70167d9
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 35 deletions.
diff --git a/src/output/config.rs b/src/output/config.rs
@@ -14,6 +14,7 @@ use crate::output::writer::{self, BufferWriter, FileWriter, StdoutWriter, Writer
 
 /// The configuration repository for the TestRun.
 pub struct Config {
+    // All fields are readable for any impl inside the crate.
     pub(crate) timestamp_provider: Box<dyn TimestampProvider + Send + Sync + 'static>,
     pub(crate) writer: WriterType,
 }
@@ -46,6 +47,7 @@ impl ConfigBuilder {
         }
     }
 
+    // TODO: docs for all these
     pub fn timezone(mut self, timezone: chrono_tz::Tz) -> Self {
         self.timestamp_provider = Box::new(ConfiguredTzProvider { tz: timezone });
         self
@@ -103,17 +105,3 @@ impl TimestampProvider for ConfiguredTzProvider {
         chrono::Local::now().with_timezone(&self.tz)
     }
 }
-
-pub struct NullTimestampProvider {}
-
-impl NullTimestampProvider {
-    // warn: linter is wrong here, this is used in a serde_json::json! block
-    #[allow(dead_code)]
-    pub const FORMATTED: &str = "1970-01-01T00:00:00.000Z";
-}
-
-impl TimestampProvider for NullTimestampProvider {
-    fn now(&self) -> chrono::DateTime<chrono_tz::Tz> {
-        chrono::DateTime::from_timestamp_nanos(0).with_timezone(&chrono_tz::UTC)
-    }
-}
diff --git a/src/output/emitter.rs b/src/output/emitter.rs
@@ -17,8 +17,7 @@ use crate::output::{
 use crate::spec;
 
 pub struct JsonEmitter {
-    // HACK: public for tests, but this should come from config directly to where needed
-    pub(crate) timestamp_provider: Box<dyn config::TimestampProvider + Send + Sync + 'static>,
+    timestamp_provider: Box<dyn config::TimestampProvider + Send + Sync + 'static>,
     writer: writer::WriterType,
     seqno: Arc<atomic::AtomicU64>,
 }
@@ -35,21 +34,26 @@ impl JsonEmitter {
         }
     }
 
-    fn serialize_artifact(&self, object: &spec::RootImpl) -> serde_json::Value {
+    fn incr_seqno(&self) -> u64 {
+        self.seqno.fetch_add(1, Ordering::AcqRel)
+    }
+
+    fn serialize_artifact(&self, object: &spec::RootImpl) -> String {
         let root = spec::Root {
             artifact: object.clone(),
             timestamp: self.timestamp_provider.now(),
             seqno: self.incr_seqno(),
         };
-        serde_json::json!(root)
+
+        serde_json::json!(root).to_string()
     }
 
-    fn incr_seqno(&self) -> u64 {
-        self.seqno.fetch_add(1, Ordering::AcqRel)
+    pub fn timestamp_provider(&self) -> &(dyn config::TimestampProvider + Send + Sync + 'static) {
+        &*self.timestamp_provider
     }
 
     pub async fn emit(&self, object: &spec::RootImpl) -> Result<(), io::Error> {
-        let s = self.serialize_artifact(object).to_string();
+        let s = self.serialize_artifact(object);
 
         match &self.writer {
             WriterType::File(file) => file.write(&s).await?,
@@ -72,6 +76,20 @@ mod tests {
 
     use super::*;
 
+    pub struct NullTimestampProvider {}
+
+    impl NullTimestampProvider {
+        // warn: linter is wrong here, this is used in a serde_json::json! block
+        #[allow(dead_code)]
+        pub const FORMATTED: &str = "1970-01-01T00:00:00.000Z";
+    }
+
+    impl config::TimestampProvider for NullTimestampProvider {
+        fn now(&self) -> chrono::DateTime<chrono_tz::Tz> {
+            chrono::DateTime::from_timestamp_nanos(0).with_timezone(&chrono_tz::UTC)
+        }
+    }
+
     #[tokio::test]
     async fn test_emit_using_buffer_writer() -> Result<()> {
         let expected = json!({
@@ -80,13 +98,13 @@ mod tests {
                 "minor": spec::SPEC_VERSION.1,
             },
             "sequenceNumber": 0,
-            "timestamp": config::NullTimestampProvider::FORMATTED,
+            "timestamp": NullTimestampProvider::FORMATTED,
         });
 
         let buffer = Arc::new(Mutex::new(vec![]));
         let writer = writer::BufferWriter::new(buffer.clone());
         let emitter = JsonEmitter::new(
-            Box::new(config::NullTimestampProvider {}),
+            Box::new(NullTimestampProvider {}),
             writer::WriterType::Buffer(writer),
         );
 
@@ -112,21 +130,21 @@ mod tests {
                 "minor": spec::SPEC_VERSION.1,
             },
             "sequenceNumber": 0,
-            "timestamp": config::NullTimestampProvider::FORMATTED,
+            "timestamp": NullTimestampProvider::FORMATTED,
         });
         let expected_2 = json!({
             "schemaVersion": {
                 "major": spec::SPEC_VERSION.0,
                 "minor": spec::SPEC_VERSION.1,
             },
             "sequenceNumber": 1,
-            "timestamp": config::NullTimestampProvider::FORMATTED,
+            "timestamp": NullTimestampProvider::FORMATTED,
         });
 
         let buffer = Arc::new(Mutex::new(vec![]));
         let writer = writer::BufferWriter::new(buffer.clone());
         let emitter = JsonEmitter::new(
-            Box::new(config::NullTimestampProvider {}),
+            Box::new(NullTimestampProvider {}),
             writer::WriterType::Buffer(writer),
         );
 

diff --git a/src/output/step.rs b/src/output/step.rs
@@ -9,10 +9,9 @@ use std::sync::atomic::{self, Ordering};
 use std::sync::Arc;
 
 use crate::output as tv;
-use crate::spec::TestStepStart;
-use crate::spec::{self, TestStepArtifactImpl};
+use crate::spec::{self, TestStepArtifactImpl, TestStepStart};
 use tv::measure::MeasurementSeries;
-use tv::{emitter, error, log, measure};
+use tv::{config, emitter, error, log, measure};
 
 use super::OcptvError;
 
@@ -31,7 +30,7 @@ impl TestStep {
             name: name.to_owned(),
             emitter: Arc::new(StepEmitter {
                 step_id: id.to_owned(),
-                run_emitter,
+                emitter: run_emitter,
             }),
         }
     }
@@ -528,7 +527,8 @@ impl StartedTestStep {
 
 pub struct StepEmitter {
     step_id: String,
-    run_emitter: Arc<emitter::JsonEmitter>,
+    // root emitter
+    emitter: Arc<emitter::JsonEmitter>,
 }
 
 impl StepEmitter {
@@ -538,13 +538,12 @@ impl StepEmitter {
             // TODO: can these copies be avoided?
             artifact: object.clone(),
         });
-        self.run_emitter.emit(&root).await?;
+        self.emitter.emit(&root).await?;
 
         Ok(())
     }
 
-    // HACK:
-    pub fn timestamp_provider(&self) -> &dyn tv::config::TimestampProvider {
-        &*self.run_emitter.timestamp_provider
+    pub fn timestamp_provider(&self) -> &(dyn config::TimestampProvider + Send + Sync + 'static) {
+        self.emitter.timestamp_provider()
     }
 }