Skip to content

Commit

Permalink
Review feedback
Browse files Browse the repository at this point in the history
- s/decade/power in more places
- Remove intermediate `LogBins` trait, just use the Histogram
  constructor
  • Loading branch information
bnaecker committed Aug 28, 2024
1 parent da16edf commit bc6f0d0
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 38 deletions.
8 changes: 4 additions & 4 deletions gateway/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,17 +40,17 @@ impl ServerContext {
};

// Track from 1 microsecond == 1e3 nanoseconds
const START_LATENCY_DECADE: u16 = 3;
const LATENCY_START_POWER: u16 = 3;
// To 1000s == 1e9 * 1e3 == 1e12 nanoseconds
const END_LATENCY_DECADE: u16 = 12;
const LATENCY_END_POWER: u16 = 12;
let latencies =
oximeter_instruments::http::LatencyTracker::with_log_linear_bins(
oximeter_instruments::http::HttpService {
name: "management-gateway-service".into(),
id,
},
START_LATENCY_DECADE,
END_LATENCY_DECADE,
LATENCY_START_POWER,
LATENCY_END_POWER,
)
.expect("start and end decades are hardcoded and should be valid");

Expand Down
8 changes: 4 additions & 4 deletions nexus/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,13 @@ impl ServerContext {
id: config.deployment.id,
};
// Start at 1 microsecond == 1e3 nanoseconds.
const START_LATENCY_DECADE: u16 = 3;
const LATENCY_START_POWER: u16 = 3;
// End at 1000s == (1e9 * 1e3) == 1e12 nanoseconds.
const END_LATENCY_DECADE: u16 = 12;
const LATENCY_END_POWER: u16 = 12;
LatencyTracker::with_log_linear_bins(
target,
START_LATENCY_DECADE,
END_LATENCY_DECADE,
LATENCY_START_POWER,
LATENCY_END_POWER,
)
.unwrap()
};
Expand Down
42 changes: 12 additions & 30 deletions oximeter/types/src/histogram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1213,40 +1213,22 @@ impl_bits!(u16);
impl_bits!(u32);
impl_bits!(u64);

/// A trait for generating logarithmically-spaced bins.
pub trait LogBins<T: HistogramSupport>: Bits {
/// Compute the left bin edges for a histogram with power-of-2 bins.
///
/// Bins start at 1, and increase in powers-of-2 until the maximum of the
/// support type.
fn power_of_two() -> Vec<T>;
}

impl<T> LogBins<T> for T
where
T: HistogramSupport + Bits,
{
fn power_of_two() -> Vec<T> {
let mut out = Vec::with_capacity(T::BITS as _);
let mut x = T::one();
out.push(x);
while let Some(next) = x.next_power() {
out.push(next);
x = next;
}
out
}
}

impl<T> Histogram<T>
where
T: LogBins<T> + HistogramSupport,
T: Bits + HistogramSupport,
{
/// Create a histogram with logarithmically spaced bins at each power of 2.
///
/// This is only available for unsigned integer support types.
pub fn power_of_two() -> Result<Self, HistogramError> {
Self::new(&T::power_of_two())
pub fn power_of_two() -> Self {
let mut bins = Vec::with_capacity(T::BITS as _);
let mut x = T::one();
bins.push(x);
while let Some(next) = x.next_power() {
bins.push(next);
x = next;
}
Self::new(&bins).expect("Bits is statically known")
}
}

Expand Down Expand Up @@ -1859,13 +1841,13 @@ mod tests {

#[test]
fn test_log_bins_u8() {
let bins = u8::power_of_two();
let (bins, _) = Histogram::<u8>::power_of_two().bins_and_counts();
assert_eq!(bins, [1, 2, 4, 8, 16, 32, 64, 128],);
}

#[test]
fn test_log_bins_u64() {
let bins = u64::power_of_two();
let (bins, _) = Histogram::<u64>::power_of_two().bins_and_counts();
for (i, bin) in bins.iter().enumerate() {
assert_eq!(*bin, 1u64 << i);
}
Expand Down

0 comments on commit bc6f0d0

Please sign in to comment.