[codex-analytics] centralize thread analytics state#20300
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffbe9fe5c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| subagent_source: Some(subagent_source_name(&input.subagent_source)), | ||
| parent_thread_id, | ||
| }); | ||
| thread_state.connection_id = parent_connection_id; |
There was a problem hiding this comment.
Preserve thread connection when recording subagent start
Avoid unconditionally overwriting thread_state.connection_id here. If SubAgentThreadStarted is ingested after emit_thread_initialized for the same thread (a common ordering), this replaces the real thread connection with the parent (or None). Later context resolution for guardian/compaction uses this field, so events can be misattributed to the wrong client/runtime or dropped entirely.
Useful? React with 👍 / 👎.
ffbe9fe to
4dae1e1
Compare
c7b6fb3 to
a89ccf9
Compare
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
owenlin0
left a comment
There was a problem hiding this comment.
i find that there are a lot of concepts to understand, can we simplify?
seems like the branch added a fairly abstract mini-framework:
- AnalyticsDropSite: where the event was dropped
- MissingAnalyticsContext: why it was dropped
- AnalyticsConnectionSource: how to find the connection
- ThreadMetadataRequirement: whether metadata is needed
- ResolvedAnalyticsContext: the resolved output
are all those necessary?
wonder if we can have a resolve_analytics_context that looks something like:
let Some((connection_state, thread_metadata)) =
self.thread_context_or_warn("compaction", &input.thread_id, Some(&input.turn_id))
else {
return;
};
or something like that
19a23ea to
b03dd51
Compare
b03dd51 to
3fc7386
Compare
3fc7386 to
d877e6a
Compare
Why
Several analytics event families need the same per-thread attribution state: the app-server client/runtime associated with a thread and, for lifecycle-oriented events, the thread metadata captured during initialization. Keeping connection ids and lifecycle metadata in separate maps made each consumer rebuild the same thread context and made subagent attribution harder to resolve consistently.
What changed
threadsmap.SubAgentThreadStartedpublication plus theSubAgentSource::ThreadSpawnparent fallback through a thread-scoped consumer that depends on inherited connection state.Verification
cargo test -p codex-analyticsStack created with Sapling. Best reviewed with ReviewStack.