[telemetry] surface projection for user message type#14374
[telemetry] surface projection for user message type#14374
Conversation
rhan-oai
force-pushed
the
rhan/surface-updates
branch
from
20497de to
9e058f1
Compare
rhan-oai
changed the title
codex-rs/protocol/src/models.rs
Outdated
| } | ||
|
|
||
| #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq, JsonSchema, TS)] | ||
| pub struct ResponseItemMetadata { |
There was a problem hiding this comment.
Can we define a unique type per ResponseItem variant? So ResponseItemMessageMetadata / ResponseItemFunctionCallOutputMetadata/ etc.?
I expect we'll have specific metadata fields for each variant (i.e. user_message_type only makes sense on Messages)
There was a problem hiding this comment.
and looking at the response item types, I think uuid is only necessary on user messages since everything else has a unique call_id
There was a problem hiding this comment.
sg on the variants, for id's uuid will be needed on tools since call_id refers to each invocation and collapses call/output which opposes item level identification
There was a problem hiding this comment.
hmm, can you explain more why call_id does not work for function calls? a call_id is 1:1 with a function call
There was a problem hiding this comment.
i think we only need to care about what codex (as a client of responsesapi) sends, not receives, so it only makes sense to attach metadata on function call outputs (not inputs/arguments, which is what responsesapi sends back to codex when the model decides to trigger a tool)
There was a problem hiding this comment.
looping in Mark to clarify deduping needs
codex-rs/protocol/src/models.rs
Outdated
| /// Client-visible UUID generated by Codex for this item. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(optional)] | ||
| pub uuid: Option<String>, |
There was a problem hiding this comment.
if we define unique metadata types, I think we could also make user_message_type and uuid non-optional
codex-rs/protocol/src/models.rs
Outdated
|
|
||
| impl ResponseItem { | ||
| /// Ensures message metadata includes a generated UUID. | ||
| pub fn with_generated_metadata_uuid(self) -> Self { |
There was a problem hiding this comment.
Instead of introducing this helper, can we just maintain this invariant when we construct a ResponseItemMessageMetadata struct? This would naturally be required once we make uuid non-optional
codex-rs/protocol/src/models.rs
Outdated
|
|
||
| impl ResponseItem { | ||
| /// Ensures message metadata includes a generated UUID. | ||
| pub fn with_generated_metadata_uuid(self) -> Self { |
There was a problem hiding this comment.
I think it would make more sense to populate this when we create the ResponseItemMetadata object, e.g. with default macro/impl or a ResponseItemMetadata::new method
There was a problem hiding this comment.
was considering this, sgtm
codex-rs/protocol/src/models.rs
Outdated
| /// Client-visible UUID generated by Codex for this item. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(optional)] | ||
| pub uuid: Option<String>, |
There was a problem hiding this comment.
let's just call this metadata_id instead of uuid - it might be slightly repetitive but we have many IDs floating around with ReponseItem
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
looking good overall! just some small comments
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, JsonSchema, TS)] | ||
| #[serde(rename_all = "snake_case")] | ||
| #[ts(rename_all = "snake_case")] | ||
| pub enum UserMessageType { |
There was a problem hiding this comment.
nit: is it worth calling this something like UserMessageSubmissionType and having these values?
Regular
Steering
Queued
seems more precise than Type
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(optional)] | ||
| pub user_message_type: Option<UserMessageType>, | ||
| /// Client-visible metadata ID generated by Codex for this item. |
There was a problem hiding this comment.
maybe worth documenting this field allows us to de-dupe
4 participants
Uh oh!
There was an error while loading. Please reload this page.