Skip to content

[APMSP-1976] Add check of parsed config and return action needed for tracer flare #1085

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
1 change: 0 additions & 1 deletion .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ datadog-trace-normalization @Datadog/serverless @Datadog/libdatadog-apm
datadog-trace-obfuscation @Datadog/serverless @Datadog/libdatadog-apm
datadog-trace-protobuf @Datadog/serverless @Datadog/libdatadog-apm
datadog-trace-utils @Datadog/serverless @Datadog/libdatadog-apm
datadog-dynamic-configuration @Datadog/libdatadog-php @Datadog/libdatadog-apm
datadog-remote-config @Datadog/libdatadog-php @Datadog/libdatadog-apm @Datadog/remote-config
datadog-sidecar @Datadog/libdatadog-php @Datadog/libdatadog-apm
datadog-sidecar-ffi @Datadog/libdatadog-php @Datadog/libdatadog-apm
Expand Down
96 changes: 65 additions & 31 deletions datadog-tracer-flare/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use std::{str::FromStr, vec};
use datadog_remote_config::{
fetch::{ConfigInvariants, SingleChangesFetcher},
file_change_tracker::{Change, FilePath},
file_storage::{ParsedFileStorage, RawFileStorage},
file_storage::{ParsedFileStorage, RawFile, RawFileStorage},
RemoteConfigData, RemoteConfigProduct, Target,
};
use ddcommon::Endpoint;
Expand All @@ -23,59 +23,92 @@ pub enum FlareError {
NoFlare(String),
/// Listening to the RemoteConfig failed.
ListeningError(String),
/// Parsing of config failed.
ParsingError(String),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be beneficial to implement Error trait for FlareError.

}

impl std::fmt::Display for FlareError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
FlareError::NoFlare(msg) => write!(f, "No flare prepared to send: {}", msg),
FlareError::ListeningError(msg) => write!(f, "Listening failed with: {}", msg),
FlareError::ParsingError(msg) => write!(f, "Parsing failed with: {}", msg),
}
}
}

/// Enum that hold the different log level possible
#[derive(Debug)]
pub enum LogLevel {
Trace = 0,
Debug = 1,
Info = 2,
Warn = 3,
Error = 4,
Critical = 5,
Off = 6,
}

/// Enum that hold the different returned action to do after listening
#[derive(Debug)]
pub enum ReturnAction {
None,
StartTrace,
StartDebug,
StartInfo,
StartWarn,
StartError,
StartCritical,
StartOff,
// The start action can be use with different level of log
StartTrace = 0,
StartDebug = 1,
StartInfo = 2,
StartWarn = 3,
StartError = 4,
StartCritical = 5,
StartOff = 6,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to use explicit discriminants here ?

Stop,
None,
}

impl From<LogLevel> for ReturnAction {
fn from(level: LogLevel) -> Self {
match level {
LogLevel::Trace => ReturnAction::StartTrace,
LogLevel::Debug => ReturnAction::StartDebug,
LogLevel::Info => ReturnAction::StartInfo,
LogLevel::Warn => ReturnAction::StartWarn,
LogLevel::Error => ReturnAction::StartError,
LogLevel::Critical => ReturnAction::StartCritical,
LogLevel::Off => ReturnAction::StartOff,
impl From<&String> for ReturnAction {
fn from(level: &String) -> Self {
match level.as_str() {
"trace" => ReturnAction::StartTrace,
"debug" => ReturnAction::StartDebug,
"info" => ReturnAction::StartInfo,
"warn" => ReturnAction::StartWarn,
"error" => ReturnAction::StartError,
"critical" => ReturnAction::StartCritical,
"off" => ReturnAction::StartOff,
_ => ReturnAction::None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we get an unknown log level is just setting it to None the appropriate action? Or should it be an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think an error would be the most appropriate. But it could be nice to launch the flare still with a default level.

}
}
}

pub type RemoteConfigFile = std::sync::Arc<RawFile<Result<RemoteConfigData, anyhow::Error>>>;
pub type Listener = SingleChangesFetcher<RawFileStorage<Result<RemoteConfigData, anyhow::Error>>>;

/// Function that check the RemoteConfig received and return the action that need to be done by the
/// tracer flare
///
/// # Arguments
///
/// * `file` - RemoteConfigFile received by the Listener.
///
/// # Returns
///
/// * `Ok(ReturnAction)` - If successful.
/// * `Ok(ReturnAction::Start<Level>)` - If AGENT_CONFIG with the right properties.
/// * `Ok(ReturnAction::Stop)` - If AGENT_TASK with the right properties.
/// * `Ok(ReturnAction::None)` - Else.
/// * `FlareError(msg)` - If something fail.
pub fn check_remote_config_file(file: RemoteConfigFile) -> Result<ReturnAction, FlareError> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this function covered by tests somewhere else? If not, anything preventing us from adding some?

let config = file.contents();
match config.as_ref() {
Ok(data) => match data {
RemoteConfigData::TracerFlareConfig(agent_config) => {
if agent_config.name.starts_with("flare-log-level.") {
if let Some(log_level) = &agent_config.config.log_level {
return Ok(log_level.into());
}
}
}
RemoteConfigData::TracerFlareTask(agent_task) => {
if agent_task.task_type.eq("tracer_flare") {
return Ok(ReturnAction::Stop);
}
}
_ => return Ok(ReturnAction::None),
},
Err(e) => {
return Err(FlareError::ParsingError(e.to_string()));
}
}
Ok(ReturnAction::None)
}

/// Function that init and return a listener of RemoteConfig
///
/// # Arguments
Expand Down Expand Up @@ -186,6 +219,7 @@ pub async fn run_remote_config_listener(
Change::Add(file) => {
println!("Added file: {} (version: {})", file.path(), file.version());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I asked this on a previous PR, but why are we printing to stdout here?

println!("Content: {:?}", file.contents().as_ref());
return check_remote_config_file(file);
}
Change::Update(file, _) => {
println!(
Expand Down
Loading