mirror of
https://github.com/openai/codex.git
synced 2026-10-21 17:48:44 +00:00
Add Guardian V2 failure reasons and connection timing metrics (#43005)
## Why Classification metrics report failures without distinguishing their causes, and WebSocket connection attempts lack a dedicated duration metric. ## What changed - Add `failure_reason` tags to classification counters and duration histograms, covering setup, sampler, and output-processing errors. - Record `codex.guardian_v2.connection.duration_ms` for WebSocket connection attempts with endpoint, outcome, and failure reason tags. - Map sampler errors to fixed, low-cardinality categories instead of using error messages that may contain server responses or credentials. GitOrigin-RevId: bb366be92af2f605643ea7cd527678fa8dbfe53f
This commit is contained in:
@@ -60,6 +60,7 @@ use super::metrics::TOOL_CALL_LAG_METRIC;
|
||||
use super::metrics::record_classification;
|
||||
use super::metrics::record_classification_risk;
|
||||
use super::metrics::record_fast_decision;
|
||||
use super::metrics::sampler_failure_reason;
|
||||
use super::review_evidence::render_review_evidence;
|
||||
use super::sampler::LunaSampler;
|
||||
use super::sampler::LunaSamplerConfig;
|
||||
@@ -541,6 +542,7 @@ impl GuardianV2Extension {
|
||||
metrics.as_deref(),
|
||||
classification_started_at.elapsed(),
|
||||
"failure",
|
||||
Some("thread_context_error"),
|
||||
);
|
||||
event_sink.emit_warning(ExtensionWarning {
|
||||
thread_id,
|
||||
@@ -586,6 +588,7 @@ impl GuardianV2Extension {
|
||||
metrics.as_deref(),
|
||||
classification_started_at.elapsed(),
|
||||
"failure",
|
||||
Some("configuration_error"),
|
||||
);
|
||||
self.event_sink.emit_warning(ExtensionWarning {
|
||||
thread_id: input.thread_store.level_id().to_owned(),
|
||||
@@ -621,6 +624,7 @@ impl GuardianV2Extension {
|
||||
metrics.as_deref(),
|
||||
classification_started_at.elapsed(),
|
||||
"skipped",
|
||||
/*failure_reason*/ None,
|
||||
);
|
||||
return;
|
||||
}
|
||||
@@ -647,6 +651,7 @@ impl GuardianV2Extension {
|
||||
metrics.as_deref(),
|
||||
classification_started_at.elapsed(),
|
||||
"failure",
|
||||
Some("parent_compaction_error"),
|
||||
);
|
||||
return;
|
||||
}
|
||||
@@ -745,6 +750,7 @@ impl GuardianV2Extension {
|
||||
metrics.as_deref(),
|
||||
classification_started_at.elapsed(),
|
||||
"failure",
|
||||
Some("context_build_error"),
|
||||
);
|
||||
event_sink.emit_warning(ExtensionWarning {
|
||||
thread_id,
|
||||
@@ -776,6 +782,7 @@ impl GuardianV2Extension {
|
||||
metrics.as_deref(),
|
||||
classification_started_at.elapsed(),
|
||||
"failure",
|
||||
Some("action_serialization_error"),
|
||||
);
|
||||
event_sink.emit_warning(ExtensionWarning {
|
||||
thread_id,
|
||||
@@ -808,6 +815,7 @@ impl GuardianV2Extension {
|
||||
format!("{planned_action}\n"),
|
||||
">>> APPROVAL REQUEST END\n".to_owned(),
|
||||
]);
|
||||
let mut failure_reason = "invalid_output";
|
||||
let mut classification_risk = None;
|
||||
let mut classification_finished_at = None;
|
||||
let result: Result<ClassificationOutcome, String> = async {
|
||||
@@ -857,7 +865,10 @@ impl GuardianV2Extension {
|
||||
Err(LunaSamplerError::Superseded) => {
|
||||
return Ok(ClassificationOutcome::Superseded);
|
||||
}
|
||||
Err(error) => return Err(error.to_string()),
|
||||
Err(error) => {
|
||||
failure_reason = sampler_failure_reason(&error);
|
||||
return Err(error.to_string());
|
||||
}
|
||||
};
|
||||
let (action_risk, risk_level) = match output.as_str() {
|
||||
"high" => (1.0, "high"),
|
||||
@@ -865,6 +876,7 @@ impl GuardianV2Extension {
|
||||
_ => return Err("invalid Guardian V2 classification".to_owned()),
|
||||
};
|
||||
classification_risk = Some(risk_level);
|
||||
failure_reason = "action_deserialization_error";
|
||||
let score = SecurityRiskScore {
|
||||
scores: BTreeMap::from([("action_risk".to_owned(), action_risk)]),
|
||||
call_id: Some(call_id.clone()),
|
||||
@@ -940,7 +952,12 @@ impl GuardianV2Extension {
|
||||
Ok(ClassificationOutcome::Superseded) => "superseded",
|
||||
Err(_) => "failure",
|
||||
};
|
||||
record_classification(metrics.as_deref(), duration, outcome);
|
||||
record_classification(
|
||||
metrics.as_deref(),
|
||||
duration,
|
||||
outcome,
|
||||
result.is_err().then_some(failure_reason),
|
||||
);
|
||||
if let Some(analytics) = analytics {
|
||||
analytics.track_guardian_v2_event(GuardianV2Event {
|
||||
thread_id: thread_id.clone(),
|
||||
|
||||
@@ -363,6 +363,9 @@ impl ExtensionMetrics for RecordingMetrics {
|
||||
}
|
||||
|
||||
fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) {
|
||||
if name == "codex.guardian_v2.connection.duration_ms" {
|
||||
return;
|
||||
}
|
||||
self.0.lock().unwrap().push(RecordedMetric::Histogram(
|
||||
name.to_owned(),
|
||||
value,
|
||||
@@ -3327,7 +3330,7 @@ async fn assert_parent_compaction_reuse(thread_context_enabled: bool) -> Result<
|
||||
sample,
|
||||
RecordedMetric::Counter(name, 1, tags)
|
||||
if name == CLASSIFICATION_METRIC
|
||||
&& tags == &[("outcome".to_owned(), "failure".to_owned())]
|
||||
&& tags.contains(&("outcome".to_owned(), "failure".to_owned()))
|
||||
)
|
||||
}));
|
||||
|
||||
|
||||
@@ -1,9 +1,13 @@
|
||||
//! Low-cardinality metrics for Guardian V2 classification and approval decisions.
|
||||
//! Low-cardinality metrics for Guardian V2 classification, connections, and approval decisions.
|
||||
|
||||
use std::time::Duration;
|
||||
|
||||
use codex_api::ApiError;
|
||||
use codex_api::TransportError;
|
||||
use codex_extension_api::ExtensionMetrics;
|
||||
|
||||
use super::sampler::LunaSamplerError;
|
||||
|
||||
pub(super) const CLASSIFICATION_METRIC: &str = "codex.guardian_v2.classification";
|
||||
pub(super) const CLASSIFICATION_DURATION_METRIC: &str =
|
||||
"codex.guardian_v2.classification.duration_ms";
|
||||
@@ -12,15 +16,58 @@ pub(super) const FAST_DECISION_METRIC: &str = "codex.guardian_v2.fast_decision";
|
||||
pub(super) const REVIEW_FALLBACK_METRIC: &str = "codex.guardian_v2.review_fallback";
|
||||
pub(super) const TOOL_CALL_LAG_METRIC: &str = "codex.guardian_v2.tool_call_lag";
|
||||
|
||||
// Never use error messages as metric tags: they may contain server responses or credentials.
|
||||
pub(super) fn sampler_failure_reason(error: &LunaSamplerError) -> &'static str {
|
||||
match error {
|
||||
LunaSamplerError::Provider(_) => "provider_error",
|
||||
LunaSamplerError::ConnectionTimeout => "connection_timeout",
|
||||
LunaSamplerError::MissingOutput => "missing_output",
|
||||
LunaSamplerError::OutputTooLarge => "output_too_large",
|
||||
LunaSamplerError::Superseded => "superseded",
|
||||
LunaSamplerError::IncompatibleCompaction => "incompatible_compaction",
|
||||
LunaSamplerError::Api(error) => match error {
|
||||
ApiError::Transport(TransportError::Http { status, .. })
|
||||
| ApiError::Api { status, .. } => match status.as_u16() {
|
||||
401 => "http_401",
|
||||
403 => "http_403",
|
||||
429 => "http_429",
|
||||
400..=499 => "http_4xx",
|
||||
500..=599 => "http_5xx",
|
||||
_ => "http_other",
|
||||
},
|
||||
ApiError::Transport(TransportError::Timeout) => "transport_timeout",
|
||||
ApiError::Transport(TransportError::Connection(_)) => "connection_error",
|
||||
ApiError::Transport(TransportError::Network(_)) => "network_error",
|
||||
ApiError::Transport(TransportError::RetryLimit) => "retry_limit",
|
||||
ApiError::Transport(TransportError::Build(_)) => "request_build_error",
|
||||
ApiError::Stream(_) => "stream_error",
|
||||
ApiError::ContextWindowExceeded => "context_window_exceeded",
|
||||
ApiError::QuotaExceeded => "quota_exceeded",
|
||||
ApiError::UsageNotIncluded => "usage_not_included",
|
||||
ApiError::Retryable { .. } => "retryable_api_error",
|
||||
ApiError::RateLimitExceeded { .. } | ApiError::RateLimit(_) => "rate_limit",
|
||||
ApiError::InvalidRequest { .. } => "invalid_request",
|
||||
ApiError::CyberPolicy { .. } | ApiError::MisalignmentPolicyViolation { .. } => {
|
||||
"policy_error"
|
||||
}
|
||||
ApiError::ServerOverloaded => "server_overloaded",
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn record_classification(
|
||||
metrics: Option<&dyn ExtensionMetrics>,
|
||||
duration: Duration,
|
||||
outcome: &str,
|
||||
failure_reason: Option<&str>,
|
||||
) {
|
||||
let Some(metrics) = metrics else {
|
||||
return;
|
||||
};
|
||||
let tags = [("outcome", outcome)];
|
||||
let mut tags = vec![("outcome", outcome)];
|
||||
if let Some(reason) = failure_reason {
|
||||
tags.push(("failure_reason", reason));
|
||||
}
|
||||
metrics.counter(CLASSIFICATION_METRIC, /*inc*/ 1, &tags);
|
||||
metrics.histogram(
|
||||
CLASSIFICATION_DURATION_METRIC,
|
||||
|
||||
@@ -46,6 +46,7 @@ use tokio::sync::Semaphore;
|
||||
use tokio::sync::oneshot;
|
||||
use uuid::Uuid;
|
||||
|
||||
use super::metrics::sampler_failure_reason;
|
||||
use super::trusted_skills::GuardianTrustedSkillsFragment;
|
||||
use super::trusted_tools::GuardianTrustedToolFragment;
|
||||
|
||||
@@ -329,10 +330,24 @@ impl LunaSampler {
|
||||
/*turn_state*/ None,
|
||||
/*telemetry*/ None,
|
||||
);
|
||||
let connection = tokio::time::timeout(provider_info.websocket_connect_timeout(), connect)
|
||||
let started_at = Instant::now();
|
||||
let result = tokio::time::timeout(provider_info.websocket_connect_timeout(), connect)
|
||||
.await
|
||||
.map_err(|_| LunaSamplerError::ConnectionTimeout)?
|
||||
.map_err(LunaSamplerError::Api)?;
|
||||
.map_err(|_| LunaSamplerError::ConnectionTimeout)
|
||||
.and_then(|result| result.map_err(LunaSamplerError::Api));
|
||||
if let Some(metrics) = self.config.metrics.as_deref() {
|
||||
let outcome = if result.is_ok() { "success" } else { "failure" };
|
||||
let mut tags = vec![("endpoint", endpoint.path()), ("outcome", outcome)];
|
||||
if let Err(error) = &result {
|
||||
tags.push(("failure_reason", sampler_failure_reason(error)));
|
||||
}
|
||||
metrics.histogram(
|
||||
"codex.guardian_v2.connection.duration_ms",
|
||||
i64::try_from(started_at.elapsed().as_millis()).unwrap_or(i64::MAX),
|
||||
&tags,
|
||||
);
|
||||
}
|
||||
let connection = result?;
|
||||
if auth_changes
|
||||
.as_ref()
|
||||
.is_some_and(|auth| auth.has_changed().unwrap_or(true))
|
||||
|
||||
@@ -216,6 +216,9 @@ impl ExtensionMetrics for RecordingMetrics {
|
||||
fn counter(&self, _name: &str, _inc: i64, _tags: &[(&str, &str)]) {}
|
||||
|
||||
fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) {
|
||||
if name == "codex.guardian_v2.connection.duration_ms" {
|
||||
return;
|
||||
}
|
||||
self.0.lock().unwrap().push((
|
||||
name.to_owned(),
|
||||
value,
|
||||
|
||||
Reference in New Issue
Block a user