From 1b5095b5d1353d94d1f884883c6acc1fe0dd9ea3 Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Fri, 2 Jan 2026 16:51:03 -0800 Subject: [PATCH] Attach more tags to feedback submissions (#8688) Attach more tags to sentry feedback so it's easier to classify and debug without having to scan through logs. Formatting isn't amazing but it's a start. image --- codex-rs/Cargo.lock | 1 + codex-rs/app-server/src/lib.rs | 10 +-- codex-rs/core/src/codex.rs | 10 +++ codex-rs/core/src/features.rs | 4 + codex-rs/core/src/util.rs | 33 +++++++ codex-rs/feedback/Cargo.toml | 1 + codex-rs/feedback/src/lib.rs | 151 +++++++++++++++++++++++++++++++++ codex-rs/tui/src/lib.rs | 11 +-- codex-rs/tui2/src/lib.rs | 11 +-- 9 files changed, 209 insertions(+), 23 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 01fa48012f..06fc14e719 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1454,6 +1454,7 @@ dependencies = [ "codex-protocol", "pretty_assertions", "sentry", + "tracing", "tracing-subscriber", ] diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 622672dc16..224e0da10b 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -17,13 +17,11 @@ use tokio::io::BufReader; use tokio::io::{self}; use tokio::sync::mpsc; use toml::Value as TomlValue; -use tracing::Level; use tracing::debug; use tracing::error; use tracing::info; use tracing_subscriber::EnvFilter; use tracing_subscriber::Layer; -use tracing_subscriber::filter::Targets; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::util::SubscriberInitExt; @@ -103,11 +101,8 @@ pub async fn run_main( .with_span_events(tracing_subscriber::fmt::format::FmtSpan::FULL) .with_filter(EnvFilter::from_default_env()); - let feedback_layer = tracing_subscriber::fmt::layer() - .with_writer(feedback.make_writer()) - .with_ansi(false) - .with_target(false) - .with_filter(Targets::new().with_default(Level::TRACE)); + let feedback_layer = feedback.logger_layer(); + let feedback_metadata_layer = feedback.metadata_layer(); let otel_logger_layer = otel.as_ref().and_then(|o| o.logger_layer()); @@ -116,6 +111,7 @@ pub async fn run_main( let _ = tracing_subscriber::registry() .with(stderr_fmt) .with(feedback_layer) + .with(feedback_metadata_layer) .with(otel_logger_layer) .with(otel_tracing_layer) .try_init(); diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 26f0a80cd1..20b9aab8fe 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -88,6 +88,7 @@ use crate::error::Result as CodexResult; #[cfg(test)] use crate::exec::StreamOutput; use crate::exec_policy::ExecPolicyUpdateError; +use crate::feedback_tags; use crate::mcp::auth::compute_auth_statuses; use crate::mcp_connection_manager::McpConnectionManager; use crate::model_provider_info::CHAT_WIRE_API_DEPRECATION_SUMMARY; @@ -2539,6 +2540,15 @@ async fn try_run_turn( truncation_policy: Some(turn_context.truncation_policy.into()), }); + feedback_tags!( + model = turn_context.client.get_model(), + approval_policy = turn_context.approval_policy, + sandbox_policy = turn_context.sandbox_policy, + effort = turn_context.client.get_reasoning_effort(), + auth_mode = sess.services.auth_manager.get_auth_mode(), + features = sess.features.enabled_features(), + ); + sess.persist_rollout_items(&[rollout_item]).await; let mut stream = turn_context .client diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 242dd38c89..3b22bfc3f4 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -255,6 +255,10 @@ impl Features { features } + + pub fn enabled_features(&self) -> Vec { + self.enabled.iter().copied().collect() + } } /// Keys accepted in `[features]` tables. diff --git a/codex-rs/core/src/util.rs b/codex-rs/core/src/util.rs index db2d0e74eb..a100f28443 100644 --- a/codex-rs/core/src/util.rs +++ b/codex-rs/core/src/util.rs @@ -9,6 +9,31 @@ use tracing::error; const INITIAL_DELAY_MS: u64 = 200; const BACKOFF_FACTOR: f64 = 2.0; +/// Emit structured feedback metadata as key/value pairs. +/// +/// This logs a tracing event with `target: "feedback_tags"`. If +/// `codex_feedback::CodexFeedback::metadata_layer()` is installed, these fields are captured and +/// later attached as tags when feedback is uploaded. +/// +/// Values are wrapped with [`tracing::field::DebugValue`], so the expression only needs to +/// implement [`std::fmt::Debug`]. +/// +/// Example: +/// +/// ```rust +/// codex_core::feedback_tags!(model = "gpt-5", cached = true); +/// codex_core::feedback_tags!(provider = provider_id, request_id = request_id); +/// ``` +#[macro_export] +macro_rules! feedback_tags { + ($( $key:ident = $value:expr ),+ $(,)?) => { + ::tracing::info!( + target: "feedback_tags", + $( $key = ::tracing::field::debug(&$value) ),+ + ); + }; +} + pub(crate) fn backoff(attempt: u64) -> Duration { let exp = BACKOFF_FACTOR.powi(attempt.saturating_sub(1) as i32); let base = (INITIAL_DELAY_MS as f64 * exp) as u64; @@ -74,4 +99,12 @@ mod tests { let message = try_parse_error_message(text); assert_eq!(message, r#"{"message": "test"}"#); } + + #[test] + fn feedback_tags_macro_compiles() { + #[derive(Debug)] + struct OnlyDebug; + + feedback_tags!(model = "gpt-5", cached = true, debug_only = OnlyDebug); + } } diff --git a/codex-rs/feedback/Cargo.toml b/codex-rs/feedback/Cargo.toml index 0ac0351333..73803af86a 100644 --- a/codex-rs/feedback/Cargo.toml +++ b/codex-rs/feedback/Cargo.toml @@ -8,6 +8,7 @@ license.workspace = true anyhow = { workspace = true } codex-protocol = { workspace = true } sentry = { version = "0.46" } +tracing = { workspace = true } tracing-subscriber = { workspace = true } [dev-dependencies] diff --git a/codex-rs/feedback/src/lib.rs b/codex-rs/feedback/src/lib.rs index eaa949717a..2096f4505b 100644 --- a/codex-rs/feedback/src/lib.rs +++ b/codex-rs/feedback/src/lib.rs @@ -1,4 +1,6 @@ +use std::collections::BTreeMap; use std::collections::VecDeque; +use std::collections::btree_map::Entry; use std::fs; use std::io::Write; use std::io::{self}; @@ -11,12 +13,20 @@ use anyhow::Result; use anyhow::anyhow; use codex_protocol::ConversationId; use codex_protocol::protocol::SessionSource; +use tracing::Event; +use tracing::Level; +use tracing::field::Visit; +use tracing_subscriber::Layer; +use tracing_subscriber::filter::Targets; use tracing_subscriber::fmt::writer::MakeWriter; +use tracing_subscriber::registry::LookupSpan; const DEFAULT_MAX_BYTES: usize = 4 * 1024 * 1024; // 4 MiB const SENTRY_DSN: &str = "https://ae32ed50620d7a7792c1ce5df38b3e3e@o33249.ingest.us.sentry.io/4510195390611458"; const UPLOAD_TIMEOUT_SECS: u64 = 10; +const FEEDBACK_TAGS_TARGET: &str = "feedback_tags"; +const MAX_FEEDBACK_TAGS: usize = 64; #[derive(Clone)] pub struct CodexFeedback { @@ -46,13 +56,50 @@ impl CodexFeedback { } } + /// Returns a [`tracing_subscriber`] layer that captures full-fidelity logs into this feedback + /// ring buffer. + /// + /// This is intended for initialization code so call sites don't have to duplicate the exact + /// `fmt::layer()` configuration and filter logic. + pub fn logger_layer(&self) -> impl Layer + Send + Sync + 'static + where + S: tracing::Subscriber + for<'a> LookupSpan<'a>, + { + tracing_subscriber::fmt::layer() + .with_writer(self.make_writer()) + .with_ansi(false) + .with_target(false) + // Capture everything, regardless of the caller's `RUST_LOG`, so feedback includes the + // full trace when the user uploads a report. + .with_filter(Targets::new().with_default(Level::TRACE)) + } + + /// Returns a [`tracing_subscriber`] layer that collects structured metadata for feedback. + /// + /// Events with `target: "feedback_tags"` are treated as key/value tags to attach to feedback + /// uploads later. + pub fn metadata_layer(&self) -> impl Layer + Send + Sync + 'static + where + S: tracing::Subscriber + for<'a> LookupSpan<'a>, + { + FeedbackMetadataLayer { + inner: self.inner.clone(), + } + .with_filter(Targets::new().with_target(FEEDBACK_TAGS_TARGET, Level::TRACE)) + } + pub fn snapshot(&self, session_id: Option) -> CodexLogSnapshot { let bytes = { let guard = self.inner.ring.lock().expect("mutex poisoned"); guard.snapshot_bytes() }; + let tags = { + let guard = self.inner.tags.lock().expect("mutex poisoned"); + guard.clone() + }; CodexLogSnapshot { bytes, + tags, thread_id: session_id .map(|id| id.to_string()) .unwrap_or("no-active-thread-".to_string() + &ConversationId::new().to_string()), @@ -62,12 +109,14 @@ impl CodexFeedback { struct FeedbackInner { ring: Mutex, + tags: Mutex>, } impl FeedbackInner { fn new(max_bytes: usize) -> Self { Self { ring: Mutex::new(RingBuffer::new(max_bytes)), + tags: Mutex::new(BTreeMap::new()), } } } @@ -152,6 +201,7 @@ impl RingBuffer { pub struct CodexLogSnapshot { bytes: Vec, + tags: BTreeMap, pub thread_id: String, } @@ -212,6 +262,22 @@ impl CodexLogSnapshot { tags.insert(String::from("reason"), r.to_string()); } + let reserved = [ + "thread_id", + "classification", + "cli_version", + "session_source", + "reason", + ]; + for (key, value) in &self.tags { + if reserved.contains(&key.as_str()) { + continue; + } + if let Entry::Vacant(entry) = tags.entry(key.clone()) { + entry.insert(value.clone()); + } + } + let level = match classification { "bug" | "bad_result" => Level::Error, _ => Level::Info, @@ -280,9 +346,80 @@ fn display_classification(classification: &str) -> String { } } +#[derive(Clone)] +struct FeedbackMetadataLayer { + inner: Arc, +} + +impl Layer for FeedbackMetadataLayer +where + S: tracing::Subscriber + for<'a> LookupSpan<'a>, +{ + fn on_event(&self, event: &Event<'_>, _ctx: tracing_subscriber::layer::Context<'_, S>) { + // This layer is filtered by `Targets`, but keep the guard anyway in case it is used without + // the filter. + if event.metadata().target() != FEEDBACK_TAGS_TARGET { + return; + } + + let mut visitor = FeedbackTagsVisitor::default(); + event.record(&mut visitor); + if visitor.tags.is_empty() { + return; + } + + let mut guard = self.inner.tags.lock().expect("mutex poisoned"); + for (key, value) in visitor.tags { + if guard.len() >= MAX_FEEDBACK_TAGS && !guard.contains_key(&key) { + continue; + } + guard.insert(key, value); + } + } +} + +#[derive(Default)] +struct FeedbackTagsVisitor { + tags: BTreeMap, +} + +impl Visit for FeedbackTagsVisitor { + fn record_i64(&mut self, field: &tracing::field::Field, value: i64) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_u64(&mut self, field: &tracing::field::Field, value: u64) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_bool(&mut self, field: &tracing::field::Field, value: bool) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_f64(&mut self, field: &tracing::field::Field, value: f64) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_str(&mut self, field: &tracing::field::Field, value: &str) { + self.tags + .insert(field.name().to_string(), value.to_string()); + } + + fn record_debug(&mut self, field: &tracing::field::Field, value: &dyn std::fmt::Debug) { + self.tags + .insert(field.name().to_string(), format!("{value:?}")); + } +} + #[cfg(test)] mod tests { use super::*; + use tracing_subscriber::layer::SubscriberExt; + use tracing_subscriber::util::SubscriberInitExt; #[test] fn ring_buffer_drops_front_when_full() { @@ -296,4 +433,18 @@ mod tests { // Capacity 8: after writing 10 bytes, we should keep the last 8. pretty_assertions::assert_eq!(std::str::from_utf8(snap.as_bytes()).unwrap(), "cdefghij"); } + + #[test] + fn metadata_layer_records_tags_from_feedback_target() { + let fb = CodexFeedback::new(); + let _guard = tracing_subscriber::registry() + .with(fb.metadata_layer()) + .set_default(); + + tracing::info!(target: FEEDBACK_TAGS_TARGET, model = "gpt-5", cached = true, "tags"); + + let snap = fb.snapshot(None); + pretty_assertions::assert_eq!(snap.tags.get("model").map(String::as_str), Some("gpt-5")); + pretty_assertions::assert_eq!(snap.tags.get("cached").map(String::as_str), Some("true")); + } } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index bce9a350f4..6b784affce 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -29,7 +29,6 @@ use std::path::PathBuf; use tracing::error; use tracing_appender::non_blocking; use tracing_subscriber::EnvFilter; -use tracing_subscriber::filter::Targets; use tracing_subscriber::prelude::*; mod additional_dirs; @@ -282,13 +281,8 @@ pub async fn run_main( .with_filter(env_filter()); let feedback = codex_feedback::CodexFeedback::new(); - let targets = Targets::new().with_default(tracing::Level::TRACE); - - let feedback_layer = tracing_subscriber::fmt::layer() - .with_writer(feedback.make_writer()) - .with_ansi(false) - .with_target(false) - .with_filter(targets); + let feedback_layer = feedback.logger_layer(); + let feedback_metadata_layer = feedback.metadata_layer(); if cli.oss && model_provider_override.is_some() { // We're in the oss section, so provider_id should be Some @@ -323,6 +317,7 @@ pub async fn run_main( let _ = tracing_subscriber::registry() .with(file_layer) .with(feedback_layer) + .with(feedback_metadata_layer) .with(otel_logger_layer) .with(otel_tracing_layer) .try_init(); diff --git a/codex-rs/tui2/src/lib.rs b/codex-rs/tui2/src/lib.rs index 4583a5d84a..015c48b442 100644 --- a/codex-rs/tui2/src/lib.rs +++ b/codex-rs/tui2/src/lib.rs @@ -29,7 +29,6 @@ use std::path::PathBuf; use tracing::error; use tracing_appender::non_blocking; use tracing_subscriber::EnvFilter; -use tracing_subscriber::filter::Targets; use tracing_subscriber::prelude::*; mod additional_dirs; @@ -292,13 +291,8 @@ pub async fn run_main( .with_filter(env_filter()); let feedback = codex_feedback::CodexFeedback::new(); - let targets = Targets::new().with_default(tracing::Level::TRACE); - - let feedback_layer = tracing_subscriber::fmt::layer() - .with_writer(feedback.make_writer()) - .with_ansi(false) - .with_target(false) - .with_filter(targets); + let feedback_layer = feedback.logger_layer(); + let feedback_metadata_layer = feedback.metadata_layer(); if cli.oss && model_provider_override.is_some() { // We're in the oss section, so provider_id should be Some @@ -333,6 +327,7 @@ pub async fn run_main( let _ = tracing_subscriber::registry() .with(file_layer) .with(feedback_layer) + .with(feedback_metadata_layer) .with(otel_tracing_layer) .with(otel_logger_layer) .try_init();