mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add auth env observability (#14905)
CXC-410 Emit Env Var Status with `/feedback` report Add more observability on top of #14611 [Unset](https://openai.sentry.io/issues/7340419168/?project=4510195390611458&query=019cfa8d-c1ba-7002-96fa-e35fc340551d&referrer=issue-stream) [Set](https://openai.sentry.io/issues/7340426331/?project=4510195390611458&query=019cfa91-aba1-7823-ab7e-762edfbc0ed4&referrer=issue-stream) <img width="1063" height="610" alt="image" src="https://github.com/user-attachments/assets/937ab026-1c2d-4757-81d5-5f31b853113e" /> ###### Summary - Adds auth-env telemetry that records whether key auth-related env overrides were present on session start and request paths. - Threads those auth-env fields through `/responses`, websocket, and `/models` telemetry and feedback metadata. - Buckets custom provider `env_key` configuration to a safe `"configured"` value instead of emitting raw config text. - Keeps the slice observability-only: no raw token values or raw URLs are emitted. ###### Rationale (from spec findings) - 401 and auth-path debugging needs a way to distinguish env-driven auth paths from sessions with no auth env override. - Startup and model-refresh failures need the same auth-env diagnostics as normal request failures. - Feedback and Sentry tags need the same auth-env signal as OTel events so reports can be triaged consistently. - Custom provider config is user-controlled text, so the telemetry contract must stay presence-only / bucketed. ###### Scope - Adds a small `AuthEnvTelemetry` bundle for env presence collection and threads it through the main request/session telemetry paths. - Does not add endpoint/base-url/provider-header/geo routing attribution or broader telemetry API redesign. ###### Trade-offs - `provider_env_key_name` is bucketed to `"configured"` instead of preserving the literal configured env var name. - `/models` is included because startup/model-refresh auth failures need the same diagnostics, but broader parity work remains out of scope. - This slice keeps the existing telemetry APIs and layers auth-env fields onto them rather than redesigning the metadata model. ###### Client follow-up - Add the separate endpoint/base-url attribution slice if routing-source diagnosis is still needed. - Add provider-header or residency attribution only if auth-env presence proves insufficient in real reports. - Revisit whether any additional auth-related env inputs need safe bucketing after more 401 triage data. ###### Testing - `cargo test -p codex-core emit_feedback_request_tags -- --nocapture` - `cargo test -p codex-core collect_auth_env_telemetry_buckets_provider_env_key_name -- --nocapture` - `cargo test -p codex-core models_request_telemetry_emits_auth_env_feedback_tags_on_failure -- --nocapture` - `cargo test -p codex-otel otel_export_routing_policy_routes_api_request_auth_observability -- --nocapture` - `cargo test -p codex-otel otel_export_routing_policy_routes_websocket_connect_auth_observability -- --nocapture` - `cargo test -p codex-otel otel_export_routing_policy_routes_websocket_request_transport_observability -- --nocapture` - `cargo test -p codex-core --no-run --message-format short` - `cargo test -p codex-otel --no-run --message-format short` --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
ee756eb80f
commit
0d2ff40a58
@@ -4,6 +4,8 @@ use crate::api_bridge::map_api_error;
|
||||
use crate::auth::AuthManager;
|
||||
use crate::auth::AuthMode;
|
||||
use crate::auth::CodexAuth;
|
||||
use crate::auth_env_telemetry::AuthEnvTelemetry;
|
||||
use crate::auth_env_telemetry::collect_auth_env_telemetry;
|
||||
use crate::config::Config;
|
||||
use crate::default_client::build_reqwest_client;
|
||||
use crate::error::CodexErr;
|
||||
@@ -15,7 +17,7 @@ use crate::models_manager::model_info;
|
||||
use crate::response_debug_context::extract_response_debug_context;
|
||||
use crate::response_debug_context::telemetry_transport_error_message;
|
||||
use crate::util::FeedbackRequestTags;
|
||||
use crate::util::emit_feedback_request_tags;
|
||||
use crate::util::emit_feedback_request_tags_with_auth_env;
|
||||
use codex_api::ModelsClient;
|
||||
use codex_api::RequestTelemetry;
|
||||
use codex_api::ReqwestTransport;
|
||||
@@ -46,6 +48,7 @@ struct ModelsRequestTelemetry {
|
||||
auth_mode: Option<String>,
|
||||
auth_header_attached: bool,
|
||||
auth_header_name: Option<&'static str>,
|
||||
auth_env: AuthEnvTelemetry,
|
||||
}
|
||||
|
||||
impl RequestTelemetry for ModelsRequestTelemetry {
|
||||
@@ -74,6 +77,12 @@ impl RequestTelemetry for ModelsRequestTelemetry {
|
||||
endpoint = MODELS_ENDPOINT,
|
||||
auth.header_attached = self.auth_header_attached,
|
||||
auth.header_name = self.auth_header_name,
|
||||
auth.env_openai_api_key_present = self.auth_env.openai_api_key_env_present,
|
||||
auth.env_codex_api_key_present = self.auth_env.codex_api_key_env_present,
|
||||
auth.env_codex_api_key_enabled = self.auth_env.codex_api_key_env_enabled,
|
||||
auth.env_provider_key_name = self.auth_env.provider_env_key_name.as_deref(),
|
||||
auth.env_provider_key_present = self.auth_env.provider_env_key_present,
|
||||
auth.env_refresh_token_url_override_present = self.auth_env.refresh_token_url_override_present,
|
||||
auth.request_id = response_debug.request_id.as_deref(),
|
||||
auth.cf_ray = response_debug.cf_ray.as_deref(),
|
||||
auth.error = response_debug.auth_error.as_deref(),
|
||||
@@ -92,28 +101,37 @@ impl RequestTelemetry for ModelsRequestTelemetry {
|
||||
endpoint = MODELS_ENDPOINT,
|
||||
auth.header_attached = self.auth_header_attached,
|
||||
auth.header_name = self.auth_header_name,
|
||||
auth.env_openai_api_key_present = self.auth_env.openai_api_key_env_present,
|
||||
auth.env_codex_api_key_present = self.auth_env.codex_api_key_env_present,
|
||||
auth.env_codex_api_key_enabled = self.auth_env.codex_api_key_env_enabled,
|
||||
auth.env_provider_key_name = self.auth_env.provider_env_key_name.as_deref(),
|
||||
auth.env_provider_key_present = self.auth_env.provider_env_key_present,
|
||||
auth.env_refresh_token_url_override_present = self.auth_env.refresh_token_url_override_present,
|
||||
auth.request_id = response_debug.request_id.as_deref(),
|
||||
auth.cf_ray = response_debug.cf_ray.as_deref(),
|
||||
auth.error = response_debug.auth_error.as_deref(),
|
||||
auth.error_code = response_debug.auth_error_code.as_deref(),
|
||||
auth.mode = self.auth_mode.as_deref(),
|
||||
);
|
||||
emit_feedback_request_tags(&FeedbackRequestTags {
|
||||
endpoint: MODELS_ENDPOINT,
|
||||
auth_header_attached: self.auth_header_attached,
|
||||
auth_header_name: self.auth_header_name,
|
||||
auth_mode: self.auth_mode.as_deref(),
|
||||
auth_retry_after_unauthorized: None,
|
||||
auth_recovery_mode: None,
|
||||
auth_recovery_phase: None,
|
||||
auth_connection_reused: None,
|
||||
auth_request_id: response_debug.request_id.as_deref(),
|
||||
auth_cf_ray: response_debug.cf_ray.as_deref(),
|
||||
auth_error: response_debug.auth_error.as_deref(),
|
||||
auth_error_code: response_debug.auth_error_code.as_deref(),
|
||||
auth_recovery_followup_success: None,
|
||||
auth_recovery_followup_status: None,
|
||||
});
|
||||
emit_feedback_request_tags_with_auth_env(
|
||||
&FeedbackRequestTags {
|
||||
endpoint: MODELS_ENDPOINT,
|
||||
auth_header_attached: self.auth_header_attached,
|
||||
auth_header_name: self.auth_header_name,
|
||||
auth_mode: self.auth_mode.as_deref(),
|
||||
auth_retry_after_unauthorized: None,
|
||||
auth_recovery_mode: None,
|
||||
auth_recovery_phase: None,
|
||||
auth_connection_reused: None,
|
||||
auth_request_id: response_debug.request_id.as_deref(),
|
||||
auth_cf_ray: response_debug.cf_ray.as_deref(),
|
||||
auth_error: response_debug.auth_error.as_deref(),
|
||||
auth_error_code: response_debug.auth_error_code.as_deref(),
|
||||
auth_recovery_followup_success: None,
|
||||
auth_recovery_followup_status: None,
|
||||
},
|
||||
&self.auth_env,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -417,11 +435,16 @@ impl ModelsManager {
|
||||
let auth_mode = auth.as_ref().map(CodexAuth::auth_mode);
|
||||
let api_provider = self.provider.to_api_provider(auth_mode)?;
|
||||
let api_auth = auth_provider_from_auth(auth.clone(), &self.provider)?;
|
||||
let auth_env = collect_auth_env_telemetry(
|
||||
&self.provider,
|
||||
self.auth_manager.codex_api_key_env_enabled(),
|
||||
);
|
||||
let transport = ReqwestTransport::new(build_reqwest_client());
|
||||
let request_telemetry: Arc<dyn RequestTelemetry> = Arc::new(ModelsRequestTelemetry {
|
||||
auth_mode: auth_mode.map(|mode| TelemetryAuthMode::from(mode).to_string()),
|
||||
auth_header_attached: api_auth.auth_header_attached(),
|
||||
auth_header_name: api_auth.auth_header_name(),
|
||||
auth_env,
|
||||
});
|
||||
let client = ModelsClient::new(transport, api_provider, api_auth)
|
||||
.with_telemetry(Some(request_telemetry));
|
||||
|
||||
@@ -3,12 +3,27 @@ use crate::CodexAuth;
|
||||
use crate::auth::AuthCredentialsStoreMode;
|
||||
use crate::config::ConfigBuilder;
|
||||
use crate::model_provider_info::WireApi;
|
||||
use base64::Engine as _;
|
||||
use chrono::Utc;
|
||||
use codex_api::TransportError;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use core_test_support::responses::mount_models_once;
|
||||
use http::HeaderMap;
|
||||
use http::StatusCode;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::collections::BTreeMap;
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
use tempfile::tempdir;
|
||||
use tracing::Event;
|
||||
use tracing::Subscriber;
|
||||
use tracing::field::Visit;
|
||||
use tracing_subscriber::Layer;
|
||||
use tracing_subscriber::layer::Context;
|
||||
use tracing_subscriber::layer::SubscriberExt;
|
||||
use tracing_subscriber::registry::LookupSpan;
|
||||
use tracing_subscriber::util::SubscriberInitExt;
|
||||
use wiremock::MockServer;
|
||||
|
||||
fn remote_model(slug: &str, display: &str, priority: i32) -> ModelInfo {
|
||||
@@ -77,6 +92,47 @@ fn provider_for(base_url: String) -> ModelProviderInfo {
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
struct TagCollectorVisitor {
|
||||
tags: BTreeMap<String, String>,
|
||||
}
|
||||
|
||||
impl Visit for TagCollectorVisitor {
|
||||
fn record_bool(&mut self, field: &tracing::field::Field, value: bool) {
|
||||
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:?}"));
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
struct TagCollectorLayer {
|
||||
tags: Arc<Mutex<BTreeMap<String, String>>>,
|
||||
}
|
||||
|
||||
impl<S> Layer<S> for TagCollectorLayer
|
||||
where
|
||||
S: Subscriber + for<'a> LookupSpan<'a>,
|
||||
{
|
||||
fn on_event(&self, event: &Event<'_>, _ctx: Context<'_, S>) {
|
||||
if event.metadata().target() != "feedback_tags" {
|
||||
return;
|
||||
}
|
||||
let mut visitor = TagCollectorVisitor::default();
|
||||
event.record(&mut visitor);
|
||||
self.tags.lock().unwrap().extend(visitor.tags);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn get_model_info_tracks_fallback_usage() {
|
||||
let codex_home = tempdir().expect("temp dir");
|
||||
@@ -530,6 +586,104 @@ async fn refresh_available_models_skips_network_without_chatgpt_auth() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn models_request_telemetry_emits_auth_env_feedback_tags_on_failure() {
|
||||
let tags = Arc::new(Mutex::new(BTreeMap::new()));
|
||||
let _guard = tracing_subscriber::registry()
|
||||
.with(TagCollectorLayer { tags: tags.clone() })
|
||||
.set_default();
|
||||
|
||||
let telemetry = ModelsRequestTelemetry {
|
||||
auth_mode: Some(TelemetryAuthMode::Chatgpt.to_string()),
|
||||
auth_header_attached: true,
|
||||
auth_header_name: Some("authorization"),
|
||||
auth_env: crate::auth_env_telemetry::AuthEnvTelemetry {
|
||||
openai_api_key_env_present: false,
|
||||
codex_api_key_env_present: false,
|
||||
codex_api_key_env_enabled: false,
|
||||
provider_env_key_name: Some("configured".to_string()),
|
||||
provider_env_key_present: Some(false),
|
||||
refresh_token_url_override_present: false,
|
||||
},
|
||||
};
|
||||
let mut headers = HeaderMap::new();
|
||||
headers.insert("x-request-id", "req-models-401".parse().unwrap());
|
||||
headers.insert("cf-ray", "ray-models-401".parse().unwrap());
|
||||
headers.insert(
|
||||
"x-openai-authorization-error",
|
||||
"missing_authorization_header".parse().unwrap(),
|
||||
);
|
||||
headers.insert(
|
||||
"x-error-json",
|
||||
base64::engine::general_purpose::STANDARD
|
||||
.encode(r#"{"error":{"code":"token_expired"}}"#)
|
||||
.parse()
|
||||
.unwrap(),
|
||||
);
|
||||
telemetry.on_request(
|
||||
1,
|
||||
Some(StatusCode::UNAUTHORIZED),
|
||||
Some(&TransportError::Http {
|
||||
status: StatusCode::UNAUTHORIZED,
|
||||
url: Some("https://example.test/models".to_string()),
|
||||
headers: Some(headers),
|
||||
body: Some("plain text error".to_string()),
|
||||
}),
|
||||
Duration::from_millis(17),
|
||||
);
|
||||
|
||||
let tags = tags.lock().unwrap().clone();
|
||||
assert_eq!(
|
||||
tags.get("endpoint").map(String::as_str),
|
||||
Some("\"/models\"")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_mode").map(String::as_str),
|
||||
Some("\"Chatgpt\"")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_request_id").map(String::as_str),
|
||||
Some("\"req-models-401\"")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_error").map(String::as_str),
|
||||
Some("\"missing_authorization_header\"")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_error_code").map(String::as_str),
|
||||
Some("\"token_expired\"")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_env_openai_api_key_present")
|
||||
.map(String::as_str),
|
||||
Some("false")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_env_codex_api_key_present")
|
||||
.map(String::as_str),
|
||||
Some("false")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_env_codex_api_key_enabled")
|
||||
.map(String::as_str),
|
||||
Some("false")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_env_provider_key_name").map(String::as_str),
|
||||
Some("\"configured\"")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_env_provider_key_present")
|
||||
.map(String::as_str),
|
||||
Some("\"false\"")
|
||||
);
|
||||
assert_eq!(
|
||||
tags.get("auth_env_refresh_token_url_override_present")
|
||||
.map(String::as_str),
|
||||
Some("false")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_available_models_picks_default_after_hiding_hidden_models() {
|
||||
let codex_home = tempdir().expect("temp dir");
|
||||
|
||||
Reference in New Issue
Block a user