Address review comments

Author: geo2a

cardano-node/src/Cardano/Node/Configuration/POM.hs

@@ -488,16 +488,13 @@ instance FromJSON PartialNodeConfiguration where
               Nothing -> return Nothing
 
       parseLedgerDbConfig v = do
-        -- TODO maybe don't silently convert old format (which was in seconds)
-        -- to new format (which is in slots), despite these being the same on
-        -- mainnet?
-        let snapInterval x = do
+        let snapIntervalSlots x = do
               si <- x .:? "SnapshotInterval"
               when (any (<= 0) si) $ fail $ "Non-positive SnapshotInterval: " <> show si
               pure $ Override . SlotNo <$> si
             snapNum x      = fmap (Override . NumOfDiskSnapshots) <$> x .:? "NumOfDiskSnapshots"
 
-        mTopLevelSnapInterval <- snapInterval v
+        mTopLevelSnapInterval <- snapIntervalSlots v
         mTopLevelSnapNum <- snapNum v
 
         let topLevelOptionsSet =
@@ -523,7 +520,7 @@ instance FromJSON PartialNodeConfiguration where
              -- Parse snapshot options from the "Snapshots" sub-object if present,
              -- otherwise fall back to the LedgerDB object for backward compatibility.
              let parseSnapshotOpts s = do
-                   sInterval  <- (getLast . (Last mTopLevelSnapInterval <>) . Last <$> snapInterval s) .!= UseDefault
+                   sInterval  <- (getLast . (Last mTopLevelSnapInterval <>) . Last <$> snapIntervalSlots s) .!= UseDefault
                    sNum       <- (getLast . (Last mTopLevelSnapNum <>) . Last <$> snapNum s)           .!= UseDefault
                    sOffset    <- (fmap Override <$> s .:? "SlotOffset") .!= UseDefault
                    sRateLimit <- (fmap (Override . secondsToDiffTime) <$> s .:? "RateLimit") .!= UseDefault

cardano-node/src/Cardano/Node/Tracing/Tracers/ChainDB.hs

@@ -1796,13 +1796,13 @@ instance ( StandardHash blk
             , "snapshot" .= forMachine dtals snap
             , "failure" .= show failure ]
   forMachine _dtals (LedgerDB.SnapshotRequestDelayed snapshotRequestTime delayBeforeSnapshotting slots) =
-    mconcat [ "kind" .= String "TraceLedgerDBEvent.LedgerDBSnapshotEvent.SnapshotRequestDelayed"
+    mconcat [ "kind" .= String "SnapshotRequestDelayed"
              , "requestTime" .= show snapshotRequestTime
-             , "delayBeforeSnapshotting " .= show delayBeforeSnapshotting
-             , "slots" .= show slots
+             , "delayBeforeSnapshotting" .= show delayBeforeSnapshotting
+             , "slots" .= toJSON (NonEmpty.toList slots)
              ]
   forMachine _dtals (LedgerDB.SnapshotRequestCompleted) =
-    mconcat [ "kind" .= String "TraceLedgerDBEvent.LedgerDBSnapshotEvent.SnapshotRequestCompleted"
+    mconcat [ "kind" .= String "SnapshotRequestCompleted"
              ]
 
 
@@ -1816,6 +1816,8 @@ instance MetaTrace (LedgerDB.TraceSnapshotEvent blk) where
     severityFor  (Namespace _ ["TookSnapshot"]) _ = Just Info
     severityFor  (Namespace _ ["DeletedSnapshot"]) _ = Just Debug
     severityFor  (Namespace _ ["InvalidSnapshot"]) _ = Just Error
+    severityFor  (Namespace _ ["SnapshotRequestDelayed"]) _ = Just Debug
+    severityFor  (Namespace _ ["SnapshotRequestCompleted"]) _ = Just Debug
     severityFor _ _ = Nothing
 
     documentFor (Namespace _ ["TookSnapshot"]) = Just $ mconcat
@@ -1840,6 +1842,8 @@ instance MetaTrace (LedgerDB.TraceSnapshotEvent blk) where
       [ Namespace [] ["TookSnapshot"]
       , Namespace [] ["DeletedSnapshot"]
       , Namespace [] ["InvalidSnapshot"]
+      , Namespace [] ["SnapshotRequestDelayed"]
+      , Namespace [] ["SnapshotRequestCompleted"]
       ]
 
 --------------------------------------------------------------------------------

configuration/cardano/mainnet-config.yaml

@@ -91,7 +91,7 @@ LedgerDB:
   # Instead of an object with individual options, a predefined snapshot
   # policy can be selected by name, e.g. `Snapshots: Mithril`.
   Snapshots:
-    # The time interval between snapshots, in seconds.
+    # The time interval between snapshots, in slots.
     SnapshotInterval: 4320
 
     # The number of disk snapshots to keep.