Skip to content

Do not record streaming measurement for quotes#4593

Merged
jmg-duarte merged 4 commits into
mainfrom
jmgd/quote
Jul 8, 2026
Merged

Do not record streaming measurement for quotes#4593
jmg-duarte merged 4 commits into
mainfrom
jmgd/quote

Conversation

@jmg-duarte

Copy link
Copy Markdown
Contributor

Description

With the new streaming measurements we started recording quotes too, this skews the metrics, the PR fixes it.
It also brings back the http_body logging that was lost when moved to the observe crate.

Changes

  • Add metrics parameter that enables the metrics recording
  • Add the http_body filter to the default log filter

How to test

Grafana

@jmg-duarte
jmg-duarte requested a review from a team as a code owner July 3, 2026 16:12
@claude

claude Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @jmg-duarte's task in 1m 43s —— View job


PR Review: Do not record streaming measurement for quotes

  • Read PR changes and understand context
  • Review streaming/mod.rs metrics changes
  • Review config/arguments changes
  • Post review findings

The change is clean and does what it says. The metrics: Option<&'static Metrics> threading is a neat way to gate record_auction_overhead so quotes (via stream_body) no longer skew the serialize_request overhead metric, while real auctions (via stream_body_and_gzip) still record it. The observe::http_body=debug filter addition correctly re-enables the transmission-timing logs — note this applies to quotes too, but that's log-only, not metrics, and appears intended.

One thing worth considering (posted inline): the metric decision is now coupled to the archiving path rather than directly to quote-vs-auction, so disabling archiving would silently drop the auction metric as well. Minor and likely fine in practice.

Nothing blocking. LGTM.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the default log filter to include observe::http_body=debug in both shared.rs and arguments.rs. Additionally, it refactors the streaming logic in crates/driver/src/infra/solver/streaming/mod.rs by introducing an optional metrics parameter to stream_into. This allows the caller to conditionally record auction overhead metrics, specifically disabling them for quotes while keeping them enabled for proper auctions. I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread crates/driver/src/infra/solver/streaming/mod.rs Outdated
Comment thread crates/shared/src/arguments.rs
Comment thread crates/driver/src/infra/solver/streaming/mod.rs Outdated
@jmg-duarte
jmg-duarte requested a review from MartinquaXD July 7, 2026 09:47

@MartinquaXD MartinquaXD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think there is 1 more metric that would be super nice to have.
Feel free to merge as is if you don't want to increase the scope of this PR.

pub struct Measurements {
/// Wall-clock serialization cost, isolated from the time spent blocked
/// writing to the sinks (solver transfer + gzip).
pub serialize: Duration,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please also record the total time and measure that for each solver individually?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, please take a look and let me know if it is what you were looking for

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, that's exactly what I had in mind. This will be very helpful to help individual solvers resolve bottlenecks.

@jmg-duarte
jmg-duarte added this pull request to the merge queue Jul 8, 2026
Merged via the queue into main with commit 72e1fbe Jul 8, 2026
22 checks passed
@jmg-duarte
jmg-duarte deleted the jmgd/quote branch July 8, 2026 09:10
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 8, 2026
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