mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Record metrics for empty skill catalogs (#35607)
## Why Empty rendered skill catalogs skipped catalog telemetry instead of reporting zero-valued counts. ## What changed - Record a default render report when thread, turn-input, or executor catalog rendering produces no fragment. - Record zero-valued host world-state metrics when skill instructions are enabled but no host skills are available. - Preserve the `catalog_surface` tag so empty host and executor catalogs remain distinguishable. ## Testing Add coverage for empty catalog rendering and mixed nonempty-executor, empty-host world state. GitOrigin-RevId: f68d65806703c0ded1ece1e5ae1ffaf6d6045126
This commit is contained in:
1 parent
d6ea5991e7
commit
16d7daad7c
5 files changed
+243
-27
No files matched your search
@@ -46,6 +46,7 @@ use crate::render::MAX_SKILL_NAME_BYTES;
|
|||||||
use crate::render::MAX_SKILL_PATH_BYTES;
|
use crate::render::MAX_SKILL_PATH_BYTES;
|
||||||
use crate::render::SkillCatalogRenderPolicy;
|
use crate::render::SkillCatalogRenderPolicy;
|
||||||
use crate::render::SkillMetadataBudget;
|
use crate::render::SkillMetadataBudget;
|
||||||
|
use crate::render::SkillRenderReport;
|
||||||
use crate::render::capped_skill_metadata_budget;
|
use crate::render::capped_skill_metadata_budget;
|
||||||
use crate::render::render_available_skills;
|
use crate::render::render_available_skills;
|
||||||
use crate::render::truncate_main_prompt_contents;
|
use crate::render::truncate_main_prompt_contents;
|
||||||
@@ -87,6 +88,12 @@ fn render_catalog(
|
|||||||
budget: SkillMetadataBudget,
|
budget: SkillMetadataBudget,
|
||||||
) -> RenderedCatalog {
|
) -> RenderedCatalog {
|
||||||
let Some(rendered) = render_available_skills(catalog, policy, budget) else {
|
let Some(rendered) = render_available_skills(catalog, policy, budget) else {
|
||||||
|
record_catalog_render(
|
||||||
|
extension_metrics,
|
||||||
|
catalog_surface,
|
||||||
|
budget,
|
||||||
|
&SkillRenderReport::default(),
|
||||||
|
);
|
||||||
return RenderedCatalog::default();
|
return RenderedCatalog::default();
|
||||||
};
|
};
|
||||||
record_catalog_render(extension_metrics, catalog_surface, budget, &rendered.report);
|
record_catalog_render(extension_metrics, catalog_surface, budget, &rendered.report);
|
||||||
@@ -98,6 +105,10 @@ fn render_catalog(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
#[path = "extension_tests.rs"]
|
||||||
|
mod tests;
|
||||||
|
|
||||||
impl<C> ThreadLifecycleContributor<C> for SkillsExtension<C>
|
impl<C> ThreadLifecycleContributor<C> for SkillsExtension<C>
|
||||||
where
|
where
|
||||||
C: Send + Sync + 'static,
|
C: Send + Sync + 'static,
|
||||||
|
|||||||
@@ -0,0 +1,57 @@
|
|||||||
|
use std::sync::Mutex;
|
||||||
|
|
||||||
|
use codex_extension_api::ExtensionMetrics;
|
||||||
|
use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC;
|
||||||
|
use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC;
|
||||||
|
use codex_otel::THREAD_SKILLS_KEPT_TOTAL_METRIC;
|
||||||
|
use codex_otel::THREAD_SKILLS_TRUNCATED_METRIC;
|
||||||
|
use pretty_assertions::assert_eq;
|
||||||
|
|
||||||
|
use super::*;
|
||||||
|
|
||||||
|
#[derive(Default)]
|
||||||
|
struct RecordingMetrics {
|
||||||
|
samples: Mutex<Vec<(String, i64)>>,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl ExtensionMetrics for RecordingMetrics {
|
||||||
|
fn histogram(&self, name: &str, value: i64, _tags: &[(&str, &str)]) {
|
||||||
|
self.samples
|
||||||
|
.lock()
|
||||||
|
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||||
|
.push((name.to_string(), value));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn empty_catalog_records_zero_metrics_without_a_fragment() {
|
||||||
|
let metrics = RecordingMetrics::default();
|
||||||
|
|
||||||
|
let rendered = render_catalog(
|
||||||
|
Some(&metrics),
|
||||||
|
CatalogSurface::ThreadContext,
|
||||||
|
&SkillCatalog::default(),
|
||||||
|
/*include_skills_usage_instructions*/ false,
|
||||||
|
SkillCatalogRenderPolicy::ExtensionCompatible,
|
||||||
|
SkillMetadataBudget::Characters(8_000),
|
||||||
|
);
|
||||||
|
|
||||||
|
assert!(rendered.fragment.is_none());
|
||||||
|
assert_eq!(rendered.warning_message, None);
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
*metrics
|
||||||
|
.samples
|
||||||
|
.lock()
|
||||||
|
.unwrap_or_else(std::sync::PoisonError::into_inner),
|
||||||
|
vec![
|
||||||
|
(THREAD_SKILLS_ENABLED_TOTAL_METRIC.to_string(), 0),
|
||||||
|
(THREAD_SKILLS_KEPT_TOTAL_METRIC.to_string(), 0),
|
||||||
|
(THREAD_SKILLS_TRUNCATED_METRIC.to_string(), 0),
|
||||||
|
(
|
||||||
|
THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC.to_string(),
|
||||||
|
0,
|
||||||
|
),
|
||||||
|
]
|
||||||
|
);
|
||||||
|
}
|
||||||
@@ -88,7 +88,7 @@ pub(crate) enum SkillMetadataBudget {
|
|||||||
Characters(usize),
|
Characters(usize),
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
#[derive(Debug, Default, Clone, PartialEq, Eq)]
|
||||||
pub(crate) struct SkillRenderReport {
|
pub(crate) struct SkillRenderReport {
|
||||||
pub(crate) total_count: usize,
|
pub(crate) total_count: usize,
|
||||||
pub(crate) included_count: usize,
|
pub(crate) included_count: usize,
|
||||||
|
|||||||
@@ -97,7 +97,20 @@ pub(crate) fn host_skills_world_state_section(
|
|||||||
} else {
|
} else {
|
||||||
None
|
None
|
||||||
};
|
};
|
||||||
let render_report = available.as_ref().map(|available| available.report.clone());
|
let render_metrics = include_instructions.then(|| {
|
||||||
|
available
|
||||||
|
.as_ref()
|
||||||
|
.map(|available| {
|
||||||
|
let report = &available.report;
|
||||||
|
(
|
||||||
|
report.total_count,
|
||||||
|
report.included_count,
|
||||||
|
report.omitted_count,
|
||||||
|
report.truncated_description_chars,
|
||||||
|
)
|
||||||
|
})
|
||||||
|
.unwrap_or_default()
|
||||||
|
});
|
||||||
let body = available.map(|available| {
|
let body = available.map(|available| {
|
||||||
AvailableSkillsInstructions::from_available_skills(
|
AvailableSkillsInstructions::from_available_skills(
|
||||||
available,
|
available,
|
||||||
@@ -128,14 +141,16 @@ pub(crate) fn host_skills_world_state_section(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if let Some(report) = &render_report {
|
if let Some((total_count, included_count, omitted_count, truncated_description_chars)) =
|
||||||
|
render_metrics
|
||||||
|
{
|
||||||
record_catalog_metrics(
|
record_catalog_metrics(
|
||||||
extension_metrics.as_deref(),
|
extension_metrics.as_deref(),
|
||||||
CatalogSurface::HostWorldState,
|
CatalogSurface::HostWorldState,
|
||||||
report.total_count,
|
total_count,
|
||||||
report.included_count,
|
included_count,
|
||||||
report.omitted_count,
|
omitted_count,
|
||||||
report.truncated_description_chars,
|
truncated_description_chars,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
let body = match body.as_deref() {
|
let body = match body.as_deref() {
|
||||||
|
|||||||
@@ -41,6 +41,7 @@ use codex_protocol::protocol::TruncationPolicy;
|
|||||||
use codex_protocol::protocol::TurnEnvironmentSelection;
|
use codex_protocol::protocol::TurnEnvironmentSelection;
|
||||||
use codex_protocol::user_input::UserInput;
|
use codex_protocol::user_input::UserInput;
|
||||||
use codex_skills::SkillMetadata;
|
use codex_skills::SkillMetadata;
|
||||||
|
use codex_skills_extension::HostSkillProvider;
|
||||||
use codex_skills_extension::SkillProviders;
|
use codex_skills_extension::SkillProviders;
|
||||||
use codex_skills_extension::SkillsExtensionConfig;
|
use codex_skills_extension::SkillsExtensionConfig;
|
||||||
use codex_skills_extension::catalog::SkillAuthority;
|
use codex_skills_extension::catalog::SkillAuthority;
|
||||||
@@ -196,6 +197,7 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes
|
|||||||
path_to_skills_md: skill_path,
|
path_to_skills_md: skill_path,
|
||||||
scope: SkillScope::User,
|
scope: SkillScope::User,
|
||||||
plugin_id: None,
|
plugin_id: None,
|
||||||
|
remote_plugin_id: None,
|
||||||
});
|
});
|
||||||
let turn_store = ExtensionData::new("turn-1");
|
let turn_store = ExtensionData::new("turn-1");
|
||||||
turn_store.insert(HostSkillsSnapshot::new(Arc::new(outcome.clone())));
|
turn_store.insert(HostSkillsSnapshot::new(Arc::new(outcome.clone())));
|
||||||
@@ -222,19 +224,13 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes
|
|||||||
.render_diff(PreviousWorldStateSection::Absent)
|
.render_diff(PreviousWorldStateSection::Absent)
|
||||||
.is_some()
|
.is_some()
|
||||||
);
|
);
|
||||||
let expected_samples = |count| {
|
let mut expected = expected_catalog_metric_samples("executor_world_state", /*count*/ 0);
|
||||||
vec![
|
expected.extend(expected_catalog_metric_samples(
|
||||||
(THREAD_SKILLS_ENABLED_TOTAL_METRIC.to_string(), count),
|
"host_world_state",
|
||||||
(THREAD_SKILLS_KEPT_TOTAL_METRIC.to_string(), count),
|
/*count*/ 1,
|
||||||
(THREAD_SKILLS_TRUNCATED_METRIC.to_string(), 0),
|
));
|
||||||
(
|
|
||||||
THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC.to_string(),
|
|
||||||
0,
|
|
||||||
),
|
|
||||||
]
|
|
||||||
};
|
|
||||||
assert!(startup_metrics.samples().is_empty());
|
assert!(startup_metrics.samples().is_empty());
|
||||||
assert_eq!(turn_metrics.samples(), expected_samples(1));
|
assert_eq!(turn_metrics.samples(), expected);
|
||||||
|
|
||||||
let sections = registry.context_contributors()[0]
|
let sections = registry.context_contributors()[0]
|
||||||
.contribute_world_state(WorldStateContributionInput {
|
.contribute_world_state(WorldStateContributionInput {
|
||||||
@@ -254,8 +250,12 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes
|
|||||||
.render_diff(PreviousWorldStateSection::Known(&published_snapshot))
|
.render_diff(PreviousWorldStateSection::Known(&published_snapshot))
|
||||||
.is_none()
|
.is_none()
|
||||||
);
|
);
|
||||||
|
expected.extend(expected_catalog_metric_samples(
|
||||||
|
"executor_world_state",
|
||||||
|
/*count*/ 0,
|
||||||
|
));
|
||||||
assert!(startup_metrics.samples().is_empty());
|
assert!(startup_metrics.samples().is_empty());
|
||||||
assert_eq!(turn_metrics.samples(), expected_samples(1));
|
assert_eq!(turn_metrics.samples(), expected);
|
||||||
|
|
||||||
let second_skill_path =
|
let second_skill_path =
|
||||||
AbsolutePathBuf::try_from(test_codex_home().join("skills/other/SKILL.md"))?;
|
AbsolutePathBuf::try_from(test_codex_home().join("skills/other/SKILL.md"))?;
|
||||||
@@ -269,6 +269,7 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes
|
|||||||
path_to_skills_md: second_skill_path,
|
path_to_skills_md: second_skill_path,
|
||||||
scope: SkillScope::User,
|
scope: SkillScope::User,
|
||||||
plugin_id: None,
|
plugin_id: None,
|
||||||
|
remote_plugin_id: None,
|
||||||
});
|
});
|
||||||
turn_store.insert(HostSkillsSnapshot::new(Arc::new(outcome)));
|
turn_store.insert(HostSkillsSnapshot::new(Arc::new(outcome)));
|
||||||
let sections = registry.context_contributors()[0]
|
let sections = registry.context_contributors()[0]
|
||||||
@@ -289,10 +290,102 @@ async fn host_world_state_records_catalog_metrics_on_publish_and_change() -> Tes
|
|||||||
.render_diff(PreviousWorldStateSection::Known(&published_snapshot))
|
.render_diff(PreviousWorldStateSection::Known(&published_snapshot))
|
||||||
.is_some()
|
.is_some()
|
||||||
);
|
);
|
||||||
let mut published_and_changed_samples = expected_samples(1);
|
expected.extend(expected_catalog_metric_samples(
|
||||||
published_and_changed_samples.extend(expected_samples(2));
|
"executor_world_state",
|
||||||
|
/*count*/ 0,
|
||||||
|
));
|
||||||
|
expected.extend(expected_catalog_metric_samples(
|
||||||
|
"host_world_state",
|
||||||
|
/*count*/ 2,
|
||||||
|
));
|
||||||
assert!(startup_metrics.samples().is_empty());
|
assert!(startup_metrics.samples().is_empty());
|
||||||
assert_eq!(turn_metrics.samples(), published_and_changed_samples);
|
assert_eq!(turn_metrics.samples(), expected);
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn nonempty_executor_empty_host_records_catalog_metrics() -> TestResult {
|
||||||
|
let executor_provider = Arc::new(StaticSkillProvider {
|
||||||
|
catalog: SkillCatalog {
|
||||||
|
entries: vec![test_entry(
|
||||||
|
SkillSourceKind::Executor,
|
||||||
|
"env-1",
|
||||||
|
"executor/lint-fix",
|
||||||
|
"lint-fix/SKILL.md",
|
||||||
|
)],
|
||||||
|
warnings: Vec::new(),
|
||||||
|
},
|
||||||
|
read_requests: Arc::new(Mutex::new(Vec::new())),
|
||||||
|
list_calls: None,
|
||||||
|
fail_first_list: false,
|
||||||
|
});
|
||||||
|
let providers = SkillProviders::new()
|
||||||
|
.with_host_provider(Arc::new(HostSkillProvider::new()))
|
||||||
|
.with_executor_provider(executor_provider);
|
||||||
|
let mut builder = ExtensionRegistryBuilder::new();
|
||||||
|
install_with_providers(&mut builder, providers, skills_extension_config);
|
||||||
|
let registry = builder.build();
|
||||||
|
let metrics = Arc::new(RecordingMetrics::default());
|
||||||
|
let session_store = ExtensionData::new("session");
|
||||||
|
let thread_store = ExtensionData::new("thread");
|
||||||
|
let config = default_config();
|
||||||
|
registry.thread_lifecycle_contributors()[0]
|
||||||
|
.on_thread_start(ThreadStartInput {
|
||||||
|
config: &config,
|
||||||
|
session_source: &SessionSource::Cli,
|
||||||
|
persistent_thread_state_available: true,
|
||||||
|
environments: &[],
|
||||||
|
mcp_resource_client: None,
|
||||||
|
extension_metrics: None,
|
||||||
|
session_store: &session_store,
|
||||||
|
thread_store: &thread_store,
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
|
||||||
|
let selected_roots = vec![SelectedCapabilityRoot {
|
||||||
|
id: "lint-fix".to_string(),
|
||||||
|
location: CapabilityRootLocation::Environment {
|
||||||
|
environment_id: "env-1".to_string(),
|
||||||
|
path: PathUri::parse("file:///skills/lint-fix").expect("skill root URI"),
|
||||||
|
},
|
||||||
|
}];
|
||||||
|
let turn_store = ExtensionData::new("turn-1");
|
||||||
|
turn_store.insert(HostSkillsSnapshot::new(Arc::new(
|
||||||
|
SkillLoadOutcome::default(),
|
||||||
|
)));
|
||||||
|
|
||||||
|
let sections = registry.context_contributors()[0]
|
||||||
|
.contribute_world_state(WorldStateContributionInput {
|
||||||
|
thread_id: codex_protocol::ThreadId::new(),
|
||||||
|
turn_id: "turn-1",
|
||||||
|
environments: &[],
|
||||||
|
ready_selected_capability_roots: &selected_roots,
|
||||||
|
executor_capability_discovery: None,
|
||||||
|
extension_metrics: Some(metrics.clone()),
|
||||||
|
session_store: &session_store,
|
||||||
|
thread_store: &thread_store,
|
||||||
|
turn_store: &turn_store,
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
|
||||||
|
assert_eq!(sections.len(), 2);
|
||||||
|
assert!(
|
||||||
|
sections[0]
|
||||||
|
.render_diff(PreviousWorldStateSection::Absent)
|
||||||
|
.is_some()
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
sections[1]
|
||||||
|
.render_diff(PreviousWorldStateSection::Absent)
|
||||||
|
.is_none()
|
||||||
|
);
|
||||||
|
let mut expected = expected_catalog_metric_samples("executor_world_state", /*count*/ 1);
|
||||||
|
expected.extend(expected_catalog_metric_samples(
|
||||||
|
"host_world_state",
|
||||||
|
/*count*/ 0,
|
||||||
|
));
|
||||||
|
assert_eq!(metrics.samples(), expected);
|
||||||
|
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
@@ -1420,13 +1513,20 @@ impl ExtensionEventSink for ChannelEventSink {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[derive(Clone, Debug, Eq, PartialEq)]
|
||||||
|
struct RecordedHistogram {
|
||||||
|
name: String,
|
||||||
|
value: i64,
|
||||||
|
tags: Vec<(String, String)>,
|
||||||
|
}
|
||||||
|
|
||||||
#[derive(Default)]
|
#[derive(Default)]
|
||||||
struct RecordingMetrics {
|
struct RecordingMetrics {
|
||||||
samples: Mutex<Vec<(String, i64)>>,
|
samples: Mutex<Vec<RecordedHistogram>>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl RecordingMetrics {
|
impl RecordingMetrics {
|
||||||
fn samples(&self) -> Vec<(String, i64)> {
|
fn samples(&self) -> Vec<RecordedHistogram> {
|
||||||
self.samples
|
self.samples
|
||||||
.lock()
|
.lock()
|
||||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||||
@@ -1435,14 +1535,47 @@ impl RecordingMetrics {
|
|||||||
}
|
}
|
||||||
|
|
||||||
impl ExtensionMetrics for RecordingMetrics {
|
impl ExtensionMetrics for RecordingMetrics {
|
||||||
fn histogram(&self, name: &str, value: i64, _tags: &[(&str, &str)]) {
|
fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) {
|
||||||
self.samples
|
self.samples
|
||||||
.lock()
|
.lock()
|
||||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||||
.push((name.to_string(), value));
|
.push(RecordedHistogram {
|
||||||
|
name: name.to_string(),
|
||||||
|
value,
|
||||||
|
tags: tags
|
||||||
|
.iter()
|
||||||
|
.map(|(key, value)| ((*key).to_string(), (*value).to_string()))
|
||||||
|
.collect(),
|
||||||
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn expected_catalog_metric_samples(catalog_surface: &str, count: i64) -> Vec<RecordedHistogram> {
|
||||||
|
let tags = vec![("catalog_surface".to_string(), catalog_surface.to_string())];
|
||||||
|
vec![
|
||||||
|
RecordedHistogram {
|
||||||
|
name: THREAD_SKILLS_ENABLED_TOTAL_METRIC.to_string(),
|
||||||
|
value: count,
|
||||||
|
tags: tags.clone(),
|
||||||
|
},
|
||||||
|
RecordedHistogram {
|
||||||
|
name: THREAD_SKILLS_KEPT_TOTAL_METRIC.to_string(),
|
||||||
|
value: count,
|
||||||
|
tags: tags.clone(),
|
||||||
|
},
|
||||||
|
RecordedHistogram {
|
||||||
|
name: THREAD_SKILLS_TRUNCATED_METRIC.to_string(),
|
||||||
|
value: 0,
|
||||||
|
tags: tags.clone(),
|
||||||
|
},
|
||||||
|
RecordedHistogram {
|
||||||
|
name: THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC.to_string(),
|
||||||
|
value: 0,
|
||||||
|
tags,
|
||||||
|
},
|
||||||
|
]
|
||||||
|
}
|
||||||
|
|
||||||
impl SkillProvider for StaticSkillProvider {
|
impl SkillProvider for StaticSkillProvider {
|
||||||
fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> {
|
fn list(&self, _query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> {
|
||||||
let list_call = self
|
let list_call = self
|
||||||
|
|||||||
Reference in new issue
Block a user