Repository navigation
[Traces] Add error sampling with opt-in tracer-drop rescue #169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
5ba576d
5d5b911
ab8ab90
156a99f
42cfbf1
f99a229
abb5314
b895a83
6bf49b7
a6e2afe
0e91eaa
23fc576
e95e578
efad1e1
5006f9a
c88d4a9
f4fe8da
dea8b63
8d998cf
b874bf4
3fa988d
391cbfe
4f436a7
0c9cb42
c8d9f76
02671a8
c6eb0f9
e4b566f
2e184e3
acf430c
a27b25c
cef1032
4d46981
6c2277c
74b4dff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,14 +9,57 @@ use std::env; | ||||||||||
| use std::str::FromStr; | |||||||||||
| use std::sync::OnceLock; | |||||||||||
|
|
|||||||||||
| use datadog_agent_trace_sampler::{ErrorSamplerConfig, ErrorSamplerMode}; | |||||||||||
| use libdd_trace_obfuscation::obfuscation_config; | |||||||||||
| use libdd_trace_utils::config_utils::{ | |||||||||||
| read_cloud_env, trace_intake_url, trace_intake_url_prefixed, trace_stats_url, | |||||||||||
| trace_stats_url_prefixed, | |||||||||||
| }; | |||||||||||
| use libdd_trace_utils::trace_utils; | |||||||||||
| use tracing::warn; | |||||||||||
|
|
|||||||||||
| const DEFAULT_APM_RECEIVER_PORT: u16 = 8126; | |||||||||||
| /// Default error rescue budget: error traces per second, matching the Go agent's `ErrorTPS`. | |||||||||||
| const DEFAULT_ERROR_SAMPLER_TPS: f64 = 10.0; | |||||||||||
|
|
|||||||||||
| /// Parses the rate-limited error rescue budget from an optional raw TPS value. | |||||||||||
| /// | |||||||||||
| /// `DD_APM_ERROR_TPS` sets the target error traces per second, default 10. Parsed | |||||||||||
| /// as `f64`; non-finite or unparsable values warn and fall back to the default. | |||||||||||
| /// A finite value <= 0 disables rescue. | |||||||||||
| /// | |||||||||||
| /// The strategy is fixed to `RateLimited`, and `extra_sample_rate` is fixed at | |||||||||||
| /// 1.0. Startup stays operational when the optional TPS setting is malformed. | |||||||||||
| fn parse_error_sampler_config(raw: Option<&str>) -> ErrorSamplerConfig { | |||||||||||
| let mut config = ErrorSamplerConfig { | |||||||||||
| mode: ErrorSamplerMode::RateLimited, | |||||||||||
|
lucaspimentel marked this conversation as resolved.
|
|||||||||||
| target_tps: DEFAULT_ERROR_SAMPLER_TPS, | |||||||||||
| extra_sample_rate: 1.0, | |||||||||||
| }; | |||||||||||
|
|
|||||||||||
| if let Some(raw) = raw { | |||||||||||
| match raw.trim().parse::<f64>() { | |||||||||||
| Ok(tps) if tps.is_finite() => config.target_tps = tps, | |||||||||||
| _ => { | |||||||||||
| warn!( | |||||||||||
| "Invalid DD_APM_ERROR_TPS {raw:?}; using default {DEFAULT_ERROR_SAMPLER_TPS}" | |||||||||||
| ); | |||||||||||
| } | |||||||||||
| } | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| config | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| // The trace agent already constructs its own configuration. Adopting | |||||||||||
| // datadog-agent-config solely for this flag would introduce broader loader coupling. | |||||||||||
| fn error_rare_sample_tracer_drop_enabled(raw: &str) -> bool { | |||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Written by Claude, an AI assistant, for Yiming] Minor: this parser copies the shared config crate The shared config crate in this repo has the same split in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping the small local parser for now. It predates #186. Consolidating the two config types would let this code use the shared apm_features field directly, without introducing a public helper and temporary dependency wiring.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Written by Claude, an AI assistant, for Yiming] OK, merging the two config types later is a better solution than a public helper. |
|||||||||||
| let delimiter = if raw.contains(',') { ',' } else { ' ' }; | |||||||||||
| raw.split(delimiter) | |||||||||||
| .map(str::trim) | |||||||||||
| .any(|entry| entry == "error_rare_sample_tracer_drop") | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| const DEFAULT_DOGSTATSD_PORT: u16 = 8125; | |||||||||||
| const DSM_PIPELINE_STATS_ROUTE: &str = "/api/v0.1/pipeline_stats"; | |||||||||||
|
|
|||||||||||
|
|
@@ -128,6 +171,12 @@ pub struct Config { | ||||||||||
| pub additional_metric_tags_cardinality_limit: Option<usize>, | |||||||||||
| /// Whether the agent should compute trace stats | |||||||||||
| pub agent_stats_computation_enabled: bool, | |||||||||||
| /// Rate-limited error rescue settings, with the budget parsed from | |||||||||||
| /// `DD_APM_ERROR_TPS`. `extra_sample_rate` is fixed at 1.0 and not | |||||||||||
| /// configurable. See `parse_error_sampler_config` for the defaults. | |||||||||||
| pub error_sampler: ErrorSamplerConfig, | |||||||||||
| /// Opt-in eligibility for tracer-dropped errors, independent of sampler enablement. | |||||||||||
| pub error_rare_sample_tracer_drop: bool, | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| impl Config { | |||||||||||
|
|
@@ -304,6 +353,9 @@ impl Config { | ||||||||||
| agent_stats_computation_enabled: env::var("DD_AGENT_STATS_COMPUTATION_ENABLED") | |||||||||||
| .map(|val| val.to_lowercase() == "true") | |||||||||||
| .unwrap_or(true), | |||||||||||
| error_sampler: parse_error_sampler_config(env::var("DD_APM_ERROR_TPS").ok().as_deref()), | |||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Written by Claude, an AI assistant, for Yiming] The settings still differ from the Lambda extension Thank you for removing the mode setting. Two other differences from the Lambda extension remain:
Do you plan to align the two products, in this PR or in a follow-up? If the differences are intentional, can you put the reason in the PR description?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping the current SCL settings in this PR. SCL uses the Go-agent-style RateLimited sampler, with DD_APM_ERROR_TPS controlling its target and zero disabling rescue. No configuration alignment with the Lambda extension is included here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Written by Claude, an AI assistant, for Yiming] OK, the Go agent setting is a good reason. We can align the Lambda extension separately. |
|||||||||||
| error_rare_sample_tracer_drop: env::var("DD_APM_FEATURES") | |||||||||||
| .is_ok_and(|raw| error_rare_sample_tracer_drop_enabled(&raw)), | |||||||||||
| }) | |||||||||||
| } | |||||||||||
| } | |||||||||||
|
|
@@ -315,6 +367,7 @@ mod tests { | ||||||||||
| use std::collections::HashMap; | |||||||||||
|
|
|||||||||||
| use crate::config; | |||||||||||
| use datadog_agent_trace_sampler::ErrorSamplerMode; | |||||||||||
|
|
|||||||||||
| #[test] | |||||||||||
| #[serial] | |||||||||||
|
|
@@ -888,6 +941,106 @@ mod tests { | ||||||||||
| }, | |||||||||||
| ); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| #[test] | |||||||||||
| fn test_apm_features_grammar() { | |||||||||||
| let feature = "error_rare_sample_tracer_drop"; | |||||||||||
| for (raw, expected) in [ | |||||||||||
| ("".to_string(), false), | |||||||||||
| (" ".to_string(), false), | |||||||||||
| ("other".to_string(), false), | |||||||||||
| (feature.to_string(), true), | |||||||||||
| (format!(" {feature} "), true), | |||||||||||
| (format!("other {feature}"), true), | |||||||||||
| (format!("other,{feature}"), true), | |||||||||||
| (format!(" other ,, {feature} "), true), | |||||||||||
| (format!("other,{feature} extra"), false), | |||||||||||
| (format!("other {feature},extra"), false), | |||||||||||
| (format!("other\t{feature}"), false), | |||||||||||
| (format!("\t{feature}\t"), true), | |||||||||||
| (feature.to_uppercase(), false), | |||||||||||
| (format!("prefix_{feature}"), false), | |||||||||||
| (format!("{feature}_suffix"), false), | |||||||||||
| ] { | |||||||||||
| assert_eq!( | |||||||||||
| super::error_rare_sample_tracer_drop_enabled(&raw), | |||||||||||
| expected, | |||||||||||
| "{raw:?}" | |||||||||||
| ); | |||||||||||
| } | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| #[cfg(unix)] | |||||||||||
| #[test] | |||||||||||
| #[serial] | |||||||||||
| fn test_unreadable_apm_features_is_disabled() { | |||||||||||
| use std::os::unix::ffi::OsStringExt; | |||||||||||
| temp_env::with_var( | |||||||||||
| "DD_APM_FEATURES", | |||||||||||
| Some(std::ffi::OsString::from_vec(vec![0xff])), | |||||||||||
| || { | |||||||||||
| temp_env::with_vars( | |||||||||||
| [ | |||||||||||
| ("DD_API_KEY", Some("test")), | |||||||||||
| ("FUNCTIONS_EXTENSION_VERSION", Some("~4")), | |||||||||||
| ("FUNCTIONS_WORKER_RUNTIME", Some("dotnet")), | |||||||||||
| ("WEBSITE_SITE_NAME", Some("test")), | |||||||||||
| ("DD_APM_ERROR_TPS", None), | |||||||||||
| ], | |||||||||||
| || { | |||||||||||
| assert!(!config::Config::new().unwrap().error_rare_sample_tracer_drop); | |||||||||||
| }, | |||||||||||
| ); | |||||||||||
| }, | |||||||||||
| ); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| #[test] | |||||||||||
| #[serial] | |||||||||||
| fn test_error_sampler_env_is_wired() { | |||||||||||
| temp_env::with_vars( | |||||||||||
| [ | |||||||||||
| ("DD_API_KEY", Some("_not_a_real_key_")), | |||||||||||
| ("FUNCTIONS_EXTENSION_VERSION", Some("~4")), | |||||||||||
| ("FUNCTIONS_WORKER_RUNTIME", Some("dotnet")), | |||||||||||
| ("WEBSITE_SITE_NAME", Some("my-azure-function")), | |||||||||||
| ("DD_APM_FEATURES", Some("error_rare_sample_tracer_drop")), | |||||||||||
| ("DD_APM_ERROR_TPS", Some("5.5")), | |||||||||||
| ], | |||||||||||
| || { | |||||||||||
| let config = config::Config::new().unwrap(); | |||||||||||
| assert!(config.error_rare_sample_tracer_drop); | |||||||||||
| assert_eq!(config.error_sampler.target_tps, 5.5); | |||||||||||
| assert_eq!(config.error_sampler.mode, ErrorSamplerMode::RateLimited); | |||||||||||
| assert_eq!(config.error_sampler.extra_sample_rate, 1.0); | |||||||||||
| }, | |||||||||||
| ); | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| #[test] | |||||||||||
| fn test_error_sampler_tps_parsing() { | |||||||||||
| for (raw, expected_tps) in [ | |||||||||||
| (None, 10.0), | |||||||||||
| (Some("5.5"), 5.5), | |||||||||||
| (Some("0"), 0.0), | |||||||||||
| (Some("-1"), -1.0), | |||||||||||
| (Some(" 10 "), 10.0), | |||||||||||
| (Some("not_a_number"), 10.0), | |||||||||||
| (Some("inf"), 10.0), | |||||||||||
| (Some("-inf"), 10.0), | |||||||||||
| (Some("NaN"), 10.0), | |||||||||||
| (Some("1e400"), 10.0), | |||||||||||
| (Some(""), 10.0), | |||||||||||
| ] { | |||||||||||
| let error_sampler = super::parse_error_sampler_config(raw); | |||||||||||
| assert_eq!( | |||||||||||
| error_sampler.target_tps, expected_tps, | |||||||||||
| "DD_APM_ERROR_TPS {raw:?} should parse to {expected_tps}" | |||||||||||
| ); | |||||||||||
| assert_eq!(error_sampler.mode, ErrorSamplerMode::RateLimited); | |||||||||||
| assert_eq!(error_sampler.extra_sample_rate, 1.0); | |||||||||||
| } | |||||||||||
| } | |||||||||||
| } | |||||||||||
|
|
|||||||||||
| /// Test helpers for creating Config instances in tests | |||||||||||
|
|
@@ -930,6 +1083,8 @@ pub mod test_helpers { | ||||||||||
| additional_metric_tags: vec![], | |||||||||||
| additional_metric_tags_cardinality_limit: None, | |||||||||||
| agent_stats_computation_enabled: true, | |||||||||||
| error_sampler: ErrorSamplerConfig::default(), | |||||||||||
| error_rare_sample_tracer_drop: false, | |||||||||||
| } | |||||||||||
| } | |||||||||||
| } | |||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Written by Claude, an AI assistant, for Yiming]
Rescue is on by default for all users
When a user upgrades, rescue starts at 10 error traces per second. The user does not need to change a setting. The Lambda extension makes rescue opt-in. Its config comment gives the reason: the extension uses
AlwaysKeep, so it has no limit on volume. This PR has a limit, so on by default can be the right choice. But it changes how many traces each user ingests.The budget is per process. So the total increases with the number of function instances.
Can the PR description state this change in behavior, and say who approved the default? It can also tell users how to turn rescue off (
DD_APM_ERROR_TPS=0).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping rescue enabled by default with a per-process target of 10 TPS, matching the Go agent's ErrorTPS default. DD_APM_ERROR_TPS=0 disables it. The PR description already documents both settings; 10 TPS is an adaptive target, not a hard cap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Written by Claude, an AI assistant, for Yiming]
Thank you. Your point about the adaptive target is correct, and I agree with the Go agent default. My only request: add one sentence to the PR description that says this turns on error rescue for current users when they upgrade. The release notes can then use it.