Fix

getsentry · iambriccardo · Jul 22, 2024 · Jul 15, 2024 · Jul 16, 2024 · Jul 16, 2024
commit 08a6e85584f7d56a74e1c65735d7911c6905ea24
@@ -1302,10 +1302,6 @@ impl Drop for BufferService {
 
 #[cfg(test)]
 mod tests {
-    use super::*;
-    use crate::services::project_cache::SpoolHealth;
-    use crate::testutils::empty_envelope;
-    use crate::utils::MemoryStat;
     use insta::assert_debug_snapshot;
     use rand::Rng;
     use relay_system::AsyncResponse;
@@ -1316,6 +1312,12 @@ mod tests {
     use std::time::{Duration, Instant};
     use uuid::Uuid;
 
+    use crate::services::project_cache::SpoolHealth;
+    use crate::testutils::empty_envelope;
+    use crate::utils::MemoryStat;
+
+    use super::*;
+
     fn services() -> Services {
         let (project_cache, _) = mock_service("project_cache", (), |&mut (), _| {});
         let (outcome_aggregator, _) = mock_service("outcome_aggregator", (), |&mut (), _| {});

@@ -164,7 +164,9 @@ pub struct ManagedEnvelope {
 }
 
 impl ManagedEnvelope {
-    /// Computes a managed envelope from the given envelope.
+    /// Computes a managed envelope from the given envelope and binds it to the processing queue.
+    ///
+    /// To provide additional scoping, use [`ManagedEnvelope::scope`].
     pub fn new(
         envelope: Box<Envelope>,
         outcome_aggregator: Addr<TrackOutcome>,

@@ -12,29 +12,62 @@ use sysinfo::{MemoryRefreshKind, System};
 /// Count after which the [`MemoryStat`] data will be refreshed.
 const UPDATE_TIME_THRESHOLD_MS: u64 = 100;
 
+/// The representation of the current memory state.
 #[derive(Clone, Copy, Debug)]
 pub struct Memory {
+    /// Used memory.
+    ///
+    /// This measure of used memory represents the Resident Set Size (RSS) which represents the
+    /// amount of physical memory that a process has in the main memory that does not correspond
+    /// to anything on disk.
     pub used: u64,
+    /// Total memory.
     pub total: u64,
 }
 
 impl Memory {
+    /// Returns the percentage amount of used memory in the interval [0.0, 1.0].
+    ///
+    /// The percentage measurement will return 1.0 in the following edge cases:
+    /// - When total is 0
+    /// - When used / total produces a NaN
     pub fn used_percent(&self) -> f32 {
-        (self.used as f32 / self.total as f32).clamp(0.0, 1.0)
+        let used_percent = self.used as f32 / self.total as f32;
+        if used_percent.is_nan() {
+            return 1.0;
+        };
+
+        used_percent.clamp(0.0, 1.0)
     }
 }
 
+/// Inner struct that holds the latest [`Memory`] state which is polled at least every 100ms.
+///
+/// The goal of this implementation is to offer lock-free reading to any arbitrary number of threads
+/// while at the same time, reducing to the minimum the need for locking when memory stats need to
+/// be updated.
+///
+/// Because of how the implementation is designed, there is a very small chance that multiple
+/// threads are waiting on the lock that guards [`System`]. The only case in which there might be
+/// multiple threads waiting on the lock, is if a thread holds the lock for more than
+/// [`UPDATE_TIME_THRESHOLD_MS`] and a new thread comes and updates the `last_update` and tries
+/// to acquire the lock to perform another memory reading. Since the reading of [`System`] is much
+/// faster than [`UPDATE_TIME_THRESHOLD_MS`] this should not happen in the real world.
 struct Inner {
     memory: ArcSwap<Memory>,
     last_update: AtomicU64,
     reference_time: Instant,
     system: Mutex<System>,
 }
 
+/// Wrapper around [`Inner`] which hides the [`Arc`] and exposes utils method to make working with
+/// [`MemoryStat`] as opaque as possible.
 #[derive(Clone)]
 pub struct MemoryStat(Arc<Inner>);
 
 impl MemoryStat {
+    /// Creates an instance of [`MemoryStat`] and obtains the current memory readings from
+    /// [`System`].
     pub fn new() -> Self {
         // sysinfo docs suggest to use a single instance of `System` across the program.
         let mut system = System::new();
@@ -46,18 +79,22 @@ impl MemoryStat {
         }))
     }
 
+    /// Returns a copy of the most up to date [`Memory`] data.
     pub fn memory(&self) -> Memory {
         self.try_update();
         **self.0.memory.load()
     }
 
+    /// Builds a [`MemoryStatConfig`] which holds a reference to the supplied [`Config`] and the
+    /// current [`MemoryStat`].
     pub fn with_config(&self, config: Arc<Config>) -> MemoryStatConfig {
         MemoryStatConfig {
             memory_stat: self.clone(),
             config,
         }
     }
 
+    /// Refreshes the memory readings.
     fn refresh_memory(system: &mut System) -> Memory {
         system.refresh_memory_specifics(MemoryRefreshKind::new().with_ram());
         let memory = match system.cgroup_limits() {
@@ -77,6 +114,7 @@ impl MemoryStat {
         memory
     }
 
+    /// Updates the memory readings if at least [`UPDATE_TIME_THRESHOLD_MS`] has passed.
     fn try_update(&self) {
         let last_update = self.0.last_update.load(Ordering::Relaxed);
         let elapsed_time = self.0.reference_time.elapsed().as_millis() as u64;
@@ -117,21 +155,33 @@ impl Default for MemoryStat {
     }
 }
 
+/// Struct that composes a [`Config`] and [`MemoryStat`] and provides utility methods to validate
+/// whether memory is within limits.
+///
+/// The rationale behind such struct, is to be able to share across Relay the same logic for dealing
+/// with memory readings. It's decoupled from [`MemoryStat`] because it's just a layer on top that
+/// decides how memory readings are interpreted.
 #[derive(Clone, Debug)]
 pub struct MemoryStatConfig {
     pub memory_stat: MemoryStat,
     config: Arc<Config>,
 }
 
 impl MemoryStatConfig {
+    /// Returns `true` if the used percentage of memory is below the specified threshold.
     pub fn has_enough_memory_percent(&self) -> bool {
         self.memory_stat.memory().used_percent() < self.config.health_max_memory_watermark_percent()
     }
 
+    /// Returns `true` if the used memory is below the specified threshold.
     pub fn has_enough_memory_bytes(&self) -> bool {
         self.memory_stat.memory().used < self.config.health_max_memory_watermark_bytes()
     }
 
+    /// Returns `true` if the used memory is below both percentage and bytes thresholds.
+    ///
+    /// This is the function that should be mainly used for checking whether of not Relay has
+    /// enough memory.
     pub fn has_enough_memory(&self) -> bool {
         let memory = self.memory_stat.memory();
         memory.used_percent() < self.config.health_max_memory_watermark_percent()
@@ -145,6 +195,12 @@ mod tests {
     use relay_config::Config;
     use std::sync::Arc;
 
+    #[test]
+    fn test_memory_used_percent_both_0() {
+        let memory = Memory { used: 0, total: 0 };
+        assert_eq!(memory.used_percent(), 1.0);
+    }
+
     #[test]
     fn test_memory_used_percent_total_0() {
         let memory = Memory {