Skip to content

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Oct 8, 2025

Unlikely fix for #4381, but worth a shot given that #2103 changed around the same time.

@bolinfest bolinfest changed the title fix: shot in the dark fix: change log_sse_event() so it no longer takes a closure Oct 8, 2025
@bolinfest bolinfest requested review from Copilot and jif-oai October 8, 2025 16:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the log_sse_event() method in the OtelEventManager to no longer accept a closure, simplifying its interface and moving timing logic to the caller sites.

  • Simplified log_sse_event() signature by removing generic closure parameters
  • Moved timing and response handling from the method to caller sites
  • Updated all call sites to handle timing locally before calling the logging method

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
codex-rs/otel/src/otel_event_manager.rs Refactored log_sse_event() to accept response and duration directly instead of executing a closure
codex-rs/core/src/client.rs Updated call site to handle timing and pass results to simplified logging method
codex-rs/core/src/chat_completions.rs Updated call site to handle timing and pass results to simplified logging method

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


match response {
Ok(Some(Ok(ref sse))) => {
Ok(Some(Ok(sse))) => {
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the ref keyword changes the ownership semantics. The code now moves sse instead of borrowing it, which could impact performance if StreamEvent is a large struct. Consider keeping the reference to avoid unnecessary moves.

Suggested change
Ok(Some(Ok(sse))) => {
Ok(Some(Ok(ref sse))) => {

Copilot uses AI. Check for mistakes.
}
Ok(Some(Err(ref error))) => {
Ok(Some(Err(error))) => {
self.sse_event_failed(None, duration, error);
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the ref keyword changes the ownership semantics. The code now moves error instead of borrowing it, which could impact performance if the error type is large. Consider keeping the reference to avoid unnecessary moves.

Suggested change
self.sse_event_failed(None, duration, error);
self.sse_event_failed(None, duration, &error);

Copilot uses AI. Check for mistakes.
@bolinfest bolinfest marked this pull request as ready for review October 8, 2025 16:42
@bolinfest bolinfest enabled auto-merge (squash) October 8, 2025 16:42
@bolinfest bolinfest merged commit fe8122e into main Oct 8, 2025
20 checks passed
@bolinfest bolinfest deleted the pr4953 branch October 8, 2025 16:53
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants