Conversation
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
these fields are going to be super helpful to have :)
| pub turn: Turn, | ||
| /// Unix timestamp (in seconds) when the turn started. | ||
| #[ts(type = "number")] | ||
| pub created_at: i64, |
There was a problem hiding this comment.
Could we update the Turn object itself to have created_at, completed_at, and duration_ms fields? That way these are all persisted and exposed through all the various APIs like thread/read, thread/resume, etc.
That likely means we need to update EventMsg::TurnStarted and EventMsg::TurnCompleted to start storing timestamps, and unfortunately we'd probably have to also make the timestamps optional since historical turns won't have them. But I think the tradeoffs are worth it
rhan-oai
force-pushed
the
pr16638
branch
7 times, most recently
from
86a7654 to
73cdd36
Compare
rhan-oai
force-pushed
the
pr16638
branch
3 times, most recently
from
fe083e6 to
88730f6
Compare
| event_turn_id: String, | ||
| status: TurnStatus, | ||
| error: Option<TurnError>, | ||
| turn_completion_metadata: TurnCompletionMetadata, |
There was a problem hiding this comment.
nit: status and error also feel like TurnCompletionMetadata, should we lump them together?
| items: vec![], | ||
| error, | ||
| status, | ||
| created_at: None, |
There was a problem hiding this comment.
doesn't feel great that this would always be None for all turn/completed notifications. Is there a way to populate it?
| pub error: Option<TurnError>, | ||
| /// Unix timestamp (in seconds) when the turn started. | ||
| #[ts(type = "number | null")] | ||
| pub created_at: Option<i64>, |
There was a problem hiding this comment.
nit: should we rename this to be started_at? The reason is that there are two possible timestamps to be using:
- the timestamp of the
turn/startRPC call - the timestamp when the core turn operation (i.e.
Op::UserInput) actually starts - core operations are queued
And it represents 2)
| items, | ||
| error: None, | ||
| status: TurnStatus::InProgress, | ||
| created_at: None, |
There was a problem hiding this comment.
We should be populating created_at whenever we can. I see a lot of places where it's hardcoded to None 🤔
There was a problem hiding this comment.
given the semantics of started_at, it doesn't make sense to populate these in app-server before core turn start as defined in turn_timing.rs is established
| turn_id: Some(turn_id.to_string()), | ||
| reason: codex_protocol::protocol::TurnAbortReason::Interrupted, | ||
| completed_at: None, | ||
| duration_ms: None, |
There was a problem hiding this comment.
nit: these are test helpers, right? could we populate these to some non-None values to be more representative?
rhan-oai
force-pushed
the
pr16638
branch
3 times, most recently
from
bbb42eb to
12d351f
Compare
rhan-oai
force-pushed
the
pr16638
branch
9 times, most recently
from
5aec095 to
964a9a5
Compare
2 participants
started_at,completed_at,duration_ms)started_attime to TurnSummary for access at completionStack created with Sapling. Best reviewed with ReviewStack.