Conversation
| // Initialize the tracing | ||
| metrics_runtime.block_on(async { prover_tracer::setup_tracing(&config.log, version) })?; |
There was a problem hiding this comment.
Nit: Would it make sense to put the block_on machinery inside some helper function prover_tracer?
crates/prover-tracer/src/lib.rs
Outdated
| .unwrap_or_default() | ||
| .split(',') | ||
| // NOTE: limit to 10 tags to avoid exploit | ||
| .take(10) |
There was a problem hiding this comment.
Nit: make his a named constant. (Or even configuration option.)
| .split(',') | ||
| // NOTE: limit to 10 tags to avoid exploit | ||
| .take(10) | ||
| .filter_map(|tag_raw| { |
There was a problem hiding this comment.
Why filter_map rather than reporting an error on syntax issues? The setup function already produces an anyhow::Result. Possibly also for the .take(10) just above but that might be a bit unwieldy.
| let mut v = tag_raw.splitn(2, '='); | ||
| match (v.next(), v.next()) { | ||
| (Some(key), Some(value)) if !key.trim().is_empty() && !value.trim().is_empty() => { | ||
| Some(KeyValue::new( | ||
| key.trim().to_string(), | ||
| value.trim().to_string(), | ||
| )) | ||
| } | ||
| _ => None, | ||
| } |
There was a problem hiding this comment.
I think you could use split_once here:
| let mut v = tag_raw.splitn(2, '='); | |
| match (v.next(), v.next()) { | |
| (Some(key), Some(value)) if !key.trim().is_empty() && !value.trim().is_empty() => { | |
| Some(KeyValue::new( | |
| key.trim().to_string(), | |
| value.trim().to_string(), | |
| )) | |
| } | |
| _ => None, | |
| } | |
| let (key, val) = tag_raw.split_once(2, '=')?; | |
| let key = key.trim(); | |
| let val = val.trim(); | |
| if key.is_empty() || val.is_empty() { | |
| return None; | |
| } | |
| Some((key.to_string(), val.to_string())) |
There was a problem hiding this comment.
Apologies, this line in the suggestion is wrong:
let (key, val) = tag_raw.split_once(2, '=')?;should be:
let (key, val) = tag_raw.split_once('=')?;There may be other issues, I have not actually run this through the compiler 😎
| impl From<TracingLevel> for EnvFilter { | ||
| fn from(value: TracingLevel) -> Self { | ||
| EnvFilter::new(format!( | ||
| "warn,prover={value},aggkit={value},agglayer={value},pessimistic_proof={value}" |
There was a problem hiding this comment.
Where are the prover=, aggkit= etc. keys coming from? I assumed these are supposed to be full crate names but might be wrong.
| #[derive(Serialize, Debug, Clone, Default, PartialEq, Eq)] | ||
| pub enum TracingOutput { | ||
| #[default] | ||
| Stdout, | ||
| Stderr, | ||
| File(PathBuf), | ||
| Otlp, | ||
| } |
There was a problem hiding this comment.
Could something like this be used to avoid the custom deserialize below?
| #[derive(Serialize, Debug, Clone, Default, PartialEq, Eq)] | |
| pub enum TracingOutput { | |
| #[default] | |
| Stdout, | |
| Stderr, | |
| File(PathBuf), | |
| Otlp, | |
| } | |
| #[derive(Serialize, Debug, Clone, Default, PartialEq, Eq, Deserialize)] | |
| pub enum TracingOutput { | |
| #[default] | |
| Stdout, | |
| Stderr, | |
| Otlp, | |
| #[serde(untagged)] | |
| File(PathBuf), | |
| } |
| let batch_processor_config = BatchConfigBuilder::default() | ||
| .with_scheduled_delay(match std::env::var("OTLP_BATCH_SCHEDULED_DELAY") { | ||
| Ok(v) => Duration::from_millis(v.parse::<u64>().unwrap_or(5_000)), | ||
| _ => Duration::from_millis(5_000), | ||
| }) | ||
| .with_max_queue_size(match std::env::var("OTLP_BATCH_MAX_QUEUE_SIZE") { | ||
| Ok(v) => v.parse::<usize>().unwrap_or(2048), | ||
| _ => 2048, | ||
| }) | ||
| .with_max_export_batch_size( | ||
| match std::env::var("OTLP_BATCH_MAX_EXPORTER_BATCH_SIZE") { | ||
| Ok(v) => v.parse::<usize>().unwrap_or(512), | ||
| _ => 512, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Here too, the various parse calls could return an error rather than silently accept a malformed input and fabricate a value out of thin air. Same further down below this comment.
3 participants
Add open telemetry output as possible output for traces.
Fixes #227