Skip to content

Add observation and ser/de metrics for serde calls - #5320

Open
MasterPtato wants to merge 1 commit into
stack/slop-claude-opus-4-8-medium-chore-add-profiling-cargo-profile-for-heaptrack-leak-investigation-xsoysnqsfrom
stack/add-observation-and-ser-de-metrics-for-serde-calls-ztnuwvts
Open

Add observation and ser/de metrics for serde calls#5320
MasterPtato wants to merge 1 commit into
stack/slop-claude-opus-4-8-medium-chore-add-profiling-cargo-profile-for-heaptrack-leak-investigation-xsoysnqsfrom
stack/add-observation-and-ser-de-metrics-for-serde-calls-ztnuwvts

Conversation

@MasterPtato

@MasterPtato MasterPtato commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@MasterPtato

MasterPtato commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Stack for rivet-dev/rivet

Get stack: forklift get 5320
Push local edits: forklift submit
Merge when ready: forklift merge 5320

change ztnuwvts

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

This PR introduces serde/observability wrapper macros (json_to_vec!, json_from_slice!, etc.) that instrument serialization/deserialization calls with timing and byte-size Prometheus metrics, plus an observe! macro for general-purpose duration tracking. It also deletes the ~2500-line UPS broadcast simulation module (sim.rs), which appears to be a clean removal with no dangling references.


engine/packages/util/src/serde.rsjson_from_slice! and bare_from_slice! double-evaluate $value

macro_rules! json_from_slice {
    ($value:expr) => {{
        let __bind = $value;
        // ... records __bind.len() for metrics ...
        $crate::observe!(serde_json::from_slice($value))  // BUG: should be from_slice(__bind)
    }};
}

$value is evaluated twice: once when bound to __bind (to measure length) and again when passed directly to serde_json::from_slice. The same bug exists in bare_from_slice!. Compare to json_from_str!, which correctly uses __bind in the second position. Current call sites all pass simple variable references so there is no runtime impact today, but any caller that passes an expression with side effects (e.g., a channel receive or iter.next().unwrap()) would evaluate it twice. The fix is to replace $value with __bind in the observe! invocation for both macros.


engine/packages/util/src/lib.rsobserve_with! docstring shows arguments in the wrong order

The docstring example:

/// observe_with!(task(), |dt, location| {
///     if dt > Duration::from_secs(10) { ... }
/// });

...but the actual macro signature is ($cb:expr, $($tt:tt)*) — callback first, then the expression to time. The correct invocation is observe_with!(|dt, location| { ... }, task()). A developer copying the docstring example will get a compile error (or silently pass task() as the callback).


engine/packages/util/src/lib.rs — unclosed code fence in observe! docstring

The doc comment opens a second ``` (after the closing ``` of the first example block) but never closes it:

/// Supports async work.
///	Use `observe_with!` for callback.
/// ```

Rustdoc will treat everything from that ``` onward as a code block, breaking the rendered docs and likely failing cargo test --doc.


engine/packages/util/src/metrics.rs — byte-size bucket arrays should be a named constant in buckets.rs (CLAUDE.md)

SERIALIZE_SIZE and DESERIALIZE_SIZE each define an identical inline vec![16.0, 32.0, ..., 16777216.0] bucket array. The project CLAUDE.md says:

Reuse existing histogram bucket constants from engine/packages/metrics/src/buckets.rs... Add a new constant to buckets.rs only if no existing constant covers the value range.

No existing constant covers byte sizes, so a new one (e.g., BYTE_SIZE_BUCKETS) should be added to buckets.rs and referenced from both metrics. Having the same array duplicated inline in two places means a future bucket adjustment would require changes in two spots.


engine/packages/util/src/metrics.rslocation label cardinality may exceed safe bounds over time (CLAUDE.md)

The location label (file!():line!():column!()) is bounded by compile-time call sites, not runtime values, so it won't explode the way actor_id would. That said, it is not in CLAUDE.md's explicit safe-label list (pool_name, workflow_name, activity_name, etc.), and as observe! gets adopted more broadly, each new instrumented call site adds a distinct time series to OBSERVATION_DURATION and LONG_OBSERVATION_DURATION. Worth discussing with the team whether a coarser label (e.g., module name or an explicit string tag passed to the macro) would better balance observability precision against series cardinality.


engine/packages/util/src/serde.rs — copy-paste doc comments on deserialization macros

json_from_str!, json_from_slice!, and bare_from_slice! all carry the doc comment /// Wraps serde_json::to_vec with observability. — wrong in both direction (these are from_* deserializers) and format (the function named is to_vec, not from_str/from_slice). These should say Wraps serde_json::from_str, Wraps serde_json::from_slice, and Wraps serde_bare::from_slice respectively.

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review

Reviewed the diff (JSON serde metrics wrapper macros in rivet-util, migration of serde_json call sites in gasoline/cache/api-peer to the new macros, and the unrelated removal of ups-broadcast/src/sim.rs).

Bugs

json_from_slice! / bare_from_slice! double-evaluate their argument (engine/packages/util/src/serde.rs:70-79, :98-107)

macro_rules! json_from_slice {
	($value:expr) => {{
		let __bind = $value;
		$crate::metrics::DESERIALIZE_SIZE...observe(__bind.len() as f64);
		$crate::observe!(serde_json::from_slice($value))   // <- re-expands $value, not __bind
	}};
}

$value is substituted twice in the expansion (once into __bind, once directly into serde_json::from_slice(...)), whereas the sibling json_from_str! macro correctly binds once and reuses __bind. For the two current call sites (cache/req_config.rs, gasoline/src/message.rs) the argument is a plain &[u8] variable so this happens to work, but it's a latent footgun for any future caller that passes a non-trivial expression (e.g. a function call) — it would be evaluated twice, causing duplicate side effects or the length/observed-size metric being computed from a different value than what's actually deserialized. bare_from_slice! has the identical bug (currently unused, so it hasn't bitten anyone yet). Recommend using __bind in both from_slice calls instead of $value.

CLAUDE.md convention

Inline duplicate bucket array instead of a shared buckets.rs constant (engine/packages/util/src/metrics.rs:20-35)
SERIALIZE_SIZE and DESERIALIZE_SIZE both inline the exact same literal vec![16.0, 32.0, ..., 16777216.0]. Per CLAUDE.md's Metrics section: "Reuse existing histogram bucket constants from engine/packages/metrics/src/buckets.rs... Add a new constant to buckets.rs only if no existing constant covers the value range." None of the existing constants (BUCKETS, MICRO_BUCKETS, PAGE_COUNT_BUCKETS, etc.) cover a byte-size range, so a new constant is justified — but it should be added to buckets.rs (e.g. SIZE_BUCKETS) and reused by both histograms rather than duplicated inline.

Minor / consistency

  • engine/packages/gasoline/src/workflow.rs:36 (StateGuard::new) deserializes state with the generic rivet_util::observe!(serde_json::from_str::<T>(...)) instead of rivet_util::serde::json_from_str!(...), unlike every other JSON deserialize call site touched in this PR. That means this call site only gets a duration metric, not the DESERIALIZE_SIZE metric — and it's inconsistent with the paired Drop impl a few lines below, which does use json_to_raw_value! (with size metric) for the write side. Given the PR's whole point is ser/de size+duration coverage, this looks like a missed conversion.
  • Doc comments on json_from_str!, json_from_slice!, and bare_from_slice! (engine/packages/util/src/serde.rs:55, :68, :96) all say "Wraps \serde_json::to_vec` with observability."/"Wraps `serde_bare::to_vec`..."— copy-pasted fromjson_to_vec!/bare_to_vec!` and don't match what the macro actually wraps.
  • engine/packages/depot/Cargo.toml swaps util.workspace = true for rivet-util.workspace = true, but nothing in depot's source references rivet_util:: (or the old util:: alias) — this looks like an unused/leftover dependency addition unrelated to the stated PR scope.
  • The ups-broadcast/src/sim.rs removal (2509 lines) is bundled into a PR titled "Add observation and ser/de metrics for serde calls." It looks like a clean, self-contained removal (no dangling references found), but mixing a large unrelated deletion into a metrics PR makes the diff harder to review/bisect — consider splitting if it wasn't already an artifact of stacking.

Other notes (not blocking)

  • The new location histogram label is a file:line:column string generated at every one of the many call sites this PR touches. It's compile-time bounded (not user input), so it doesn't violate the "no unbounded labels" rule, but it's worth being aware this will create a fairly large number of distinct series across the codebase if observe!/json_*! usage keeps spreading — seems intentional for the heaptrack investigation this stack is for.
  • No test coverage was added for the new macros. Given the double-evaluation bug above, a simple unit test asserting json_from_slice!/bare_from_slice! only evaluate their argument once (e.g. via a call-counting closure) would have caught it.

Everything else (the mechanical serde_json::*rivet_util::serde::json_*! swaps across gasoline, cache, api-peer) looks correct and behavior-preserving.

@railway-app

railway-app Bot commented Jun 23, 2026

Copy link
Copy Markdown

🚅 Deployed to the rivet-pr-5320 environment in rivet-frontend

Service Status Web Updated (UTC)
website ✅ Success (View Logs) Web Aug 7, 2026 at 5:15 pm
frontend-inspector 😴 Sleeping (View Logs) Web Aug 7, 2026 at 1:23 am
frontend-cloud 😴 Sleeping (View Logs) Web Aug 6, 2026 at 7:04 pm
kitchen-sink 😴 Sleeping (View Logs) Web Aug 2, 2026 at 10:30 pm
ladle ✅ Success (View Logs) Web Jun 23, 2026 at 8:25 pm
mcp-hub ✅ Success (View Logs) Web Jun 23, 2026 at 8:24 pm

@MasterPtato
MasterPtato changed the base branch from stack/slop-claude-opus-4-8-medium-chore-add-profiling-cargo-profile-for-heaptrack-leak-investigation-xsoysnqs to main August 7, 2026 00:39
@MasterPtato
MasterPtato force-pushed the stack/add-observation-and-ser-de-metrics-for-serde-calls-ztnuwvts branch from a0c66b8 to 9ff6358 Compare August 7, 2026 01:27
@MasterPtato
MasterPtato changed the base branch from main to stack/slop-claude-opus-4-8-medium-chore-add-profiling-cargo-profile-for-heaptrack-leak-investigation-xsoysnqs August 7, 2026 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant