Skip to content

Commit 4f6eac6

Browse files
sshaderConvex, Inc.
authored andcommitted
Add a JSON variation of tracing sampling config (#30924)
The string format is IMO confusing now that there are both instance overrides and route overrides. The tests have a bunch of examples of what the new format looks like + its behavior. A couple interesting notes: * Named everything `fraction` + added errors if the value is ever something that's not between 0 and 1 * `defaultFraction` is technically redundant with a blanket `routeOverrides`, but it seems nice to be able to tell the default sample rate at a glance. * `instanceOverrides` is kind of dumb when setting this value for backend, but it's optional and seems not worth the work to structure differently I don't know if we have strict limits on the length of the knob value, but I'm hoping going for clarity over compactness is less error prone. Both formats will work for now (using starts with `{` as the criteria for the JSON format), and once it's rolled out without chance for rollback, I'll change any knobs + the default value across our relevant services and delete the old code path. GitOrigin-RevId: 607991e228a2b4b98cb36e95f8cc2141db831dee
1 parent af942d6 commit 4f6eac6

File tree

1 file changed

+160
-0
lines changed

1 file changed

+160
-0
lines changed

crates/common/src/minitrace_helpers.rs

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ use minitrace::{
1717
use parking_lot::Mutex;
1818
use rand::Rng;
1919
use regex::Regex;
20+
use serde::Deserialize;
2021

2122
use crate::knobs::REQUEST_TRACE_SAMPLE_CONFIG;
2223

@@ -139,10 +140,82 @@ impl SamplingConfig {
139140
}
140141
}
141142

143+
#[derive(Debug, Deserialize)]
144+
#[serde(rename_all = "camelCase")]
145+
struct RouteOverride {
146+
route_regexp: String,
147+
fraction: f64,
148+
}
149+
150+
// These are in priority order -- instance overrides take precedence over route
151+
// overrides, which take precedence over the default fraction.
152+
//
153+
// When in doubt, write out a test case to verify the behavior.
154+
// Technically the default fraction is redundant with `routeOverrides: [{
155+
// "routeRegexp": ".*", "fraction": ... }]`, but it's pulled out for clarity.
156+
#[derive(Debug, Deserialize)]
157+
#[serde(rename_all = "camelCase")]
158+
struct SamplingConfigJson {
159+
instance_overrides: Option<BTreeMap<String, Vec<RouteOverride>>>,
160+
route_overrides: Option<Vec<RouteOverride>>,
161+
default_fraction: f64,
162+
}
163+
164+
fn validate_fraction(value: f64, context: &str) -> anyhow::Result<f64> {
165+
if !(0.0..=1.0).contains(&value) {
166+
anyhow::bail!(
167+
"Invalid fraction {} in {}: clamping to [0.0, 1.0]",
168+
value,
169+
context
170+
);
171+
}
172+
Ok(value)
173+
}
174+
175+
impl TryFrom<SamplingConfigJson> for SamplingConfig {
176+
type Error = anyhow::Error;
177+
178+
fn try_from(json: SamplingConfigJson) -> anyhow::Result<Self> {
179+
let mut by_regex = Vec::new();
180+
if let Some(instance_overrides) = json.instance_overrides {
181+
for (instance_name, route_overrides) in instance_overrides.iter() {
182+
for route_override in route_overrides {
183+
by_regex.push((
184+
Some(instance_name.to_owned()),
185+
Regex::new(&route_override.route_regexp).context("Invalid route regexp")?,
186+
validate_fraction(route_override.fraction, instance_name)?,
187+
));
188+
}
189+
}
190+
}
191+
if let Some(route_overrides) = json.route_overrides {
192+
for route_override in route_overrides {
193+
by_regex.push((
194+
None,
195+
Regex::new(&route_override.route_regexp).context("Invalid route regexp")?,
196+
validate_fraction(route_override.fraction, &route_override.route_regexp)?,
197+
));
198+
}
199+
}
200+
by_regex.push((
201+
None,
202+
Regex::new(".*").expect(".* is not a valid regex"),
203+
validate_fraction(json.default_fraction, "default")?,
204+
));
205+
Ok(SamplingConfig { by_regex })
206+
}
207+
}
208+
142209
impl FromStr for SamplingConfig {
143210
type Err = anyhow::Error;
144211

145212
fn from_str(s: &str) -> anyhow::Result<Self> {
213+
if s.starts_with('{') {
214+
let json: SamplingConfigJson =
215+
serde_json::from_str(s).context("Failed to parse sampling config as JSON")?;
216+
return SamplingConfig::try_from(json);
217+
}
218+
146219
let mut by_regex = Vec::new();
147220
for token in s.split(',') {
148221
let parts: Vec<_> = token.split(':').map(|s| s.trim()).collect();
@@ -234,4 +307,91 @@ mod tests {
234307

235308
Ok(())
236309
}
310+
311+
#[test]
312+
fn test_parse_sampling_config_json() -> anyhow::Result<()> {
313+
let config: SamplingConfig = r#"{ "defaultFraction": 1.0 }"#.parse()?;
314+
assert_eq!(config.by_regex.len(), 1);
315+
assert_eq!(config.sample_ratio("carnitas", "a"), 1.0);
316+
317+
let config: SamplingConfig = r#"{
318+
"routeOverrides": [
319+
{ "routeRegexp": "a", "fraction": 0.5 },
320+
{ "routeRegexp": "b", "fraction": 0.15 }
321+
],
322+
"defaultFraction": 0.0
323+
}"#
324+
.parse()?;
325+
assert_eq!(config.by_regex.len(), 3);
326+
assert_eq!(config.sample_ratio("carnitas", "a"), 0.5);
327+
assert_eq!(config.sample_ratio("carnitas", "b"), 0.15);
328+
assert_eq!(config.sample_ratio("carnitas", "c"), 0.0);
329+
330+
let config: SamplingConfig = r#"{
331+
"routeOverrides": [
332+
{ "routeRegexp": "a", "fraction": 0.5 },
333+
{ "routeRegexp": "b", "fraction": 0.15 }
334+
],
335+
"defaultFraction": 0.01
336+
}"#
337+
.parse()?;
338+
assert_eq!(config.by_regex.len(), 3);
339+
assert_eq!(config.sample_ratio("carnitas", "a"), 0.5);
340+
assert_eq!(config.sample_ratio("carnitas", "b"), 0.15);
341+
assert_eq!(config.sample_ratio("carnitas", "c"), 0.01);
342+
343+
let config: SamplingConfig = r#"{
344+
"routeOverrides": [
345+
{ "routeRegexp": "/f/.*", "fraction": 0.5 }
346+
],
347+
"defaultFraction": 0.0
348+
}"#
349+
.parse()?;
350+
assert_eq!(config.by_regex.len(), 2);
351+
assert_eq!(config.sample_ratio("carnitas", "/f/a"), 0.5);
352+
assert_eq!(config.sample_ratio("carnitas", "/f/b"), 0.5);
353+
assert_eq!(config.sample_ratio("carnitas", "c"), 0.0);
354+
355+
// Instance overrides.
356+
let config: SamplingConfig = r#"{
357+
"instanceOverrides": {
358+
"alpastor": [ { "routeRegexp": "a", "fraction": 0.5 } ],
359+
"carnitas": [ { "routeRegexp": ".*", "fraction": 0.01 } ]
360+
},
361+
"routeOverrides": [
362+
{ "routeRegexp": "b", "fraction": 0.15 }
363+
],
364+
"defaultFraction": 1.0
365+
}"#
366+
.parse()?;
367+
assert_eq!(config.by_regex.len(), 4);
368+
assert_eq!(config.sample_ratio("carnitas", "a"), 0.01);
369+
assert_eq!(config.sample_ratio("carnitas", "b"), 0.01);
370+
assert_eq!(config.sample_ratio("carnitas", "c"), 0.01);
371+
assert_eq!(config.sample_ratio("alpastor", "a"), 0.5);
372+
assert_eq!(config.sample_ratio("alpastor", "b"), 0.15);
373+
assert_eq!(config.sample_ratio("alpastor", "c"), 1.0);
374+
assert_eq!(config.sample_ratio("chorizo", "a"), 1.0);
375+
assert_eq!(config.sample_ratio("chorizo", "b"), 0.15);
376+
assert_eq!(config.sample_ratio("chorizo", "c"), 1.0);
377+
378+
// Invalid configs.
379+
let err = "{ defaultFraction: 1.0 }"
380+
.parse::<SamplingConfig>()
381+
.unwrap_err();
382+
assert!(format!("{}", err).contains("Failed to parse sampling config as JSON"));
383+
384+
let err = r#"{ "defaultFraction": 4.0 }"#.parse::<SamplingConfig>().unwrap_err();
385+
assert!(format!("{}", err).contains("Invalid fraction 4 in default"));
386+
387+
let err = r#"{
388+
"defaultFraction": 1.0,
389+
"routeOverrides": [{ "routeRegexp": "(", "fraction": 0.5 }]
390+
}"#
391+
.parse::<SamplingConfig>()
392+
.unwrap_err();
393+
assert!(format!("{}", err).contains("Invalid route regexp"));
394+
395+
Ok(())
396+
}
237397
}

0 commit comments

Comments
 (0)