Skip to content

Add observation for serde_bare#5321

Open
MasterPtato wants to merge 1 commit into
stack/add-observation-and-ser-de-metrics-for-serde-calls-ztnuwvtsfrom
stack/add-observation-for-serde_bare-oskoutmw
Open

Add observation for serde_bare#5321
MasterPtato wants to merge 1 commit into
stack/add-observation-and-ser-de-metrics-for-serde-calls-ztnuwvtsfrom
stack/add-observation-for-serde_bare-oskoutmw

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

Current stack:

Dependencies:

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

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Code Review

Summary

This PR adds observability (latency timing + size histograms) to every raw serde_bare::from_slice / serde_bare::to_vec call site by replacing them with the rivet_util::serde::bare_from_slice! and rivet_util::serde::bare_to_vec! macros, which emit SERIALIZE_SIZE / DESERIALIZE_SIZE histogram metrics and route through the existing observe! macro for duration tracking. The gasoline StateGuard::new switch from observe!(serde_json::from_str...)? to json_from_str! is consistent with the same pattern applied to bare.

Issues Found

Minor: Two doc-comment copy-paste errors in engine/packages/util/src/serde.rs

Lines 81 and 96 both now read /// Wraps rivet_util::serde::bare_to_vec! with observability. Line 81 documents bare_to_vec correctly, but line 96 documents bare_from_slice with the wrong name. The doc comment on line 96 should say bare_from_slice! instead.

Minor: serde_bare direct dependency may be droppable in pegboard-runner after adding rivet-util

engine/packages/pegboard-runner/Cargo.toml adds rivet-util but retains the existing serde_bare direct dependency. If all serde_bare uses in that crate now go through the macro wrapper, the direct dependency can be dropped to prevent future callsites accidentally bypassing the observability wrapper.

What Looks Good

  • Every OwnedVersionedData impl in the protocol crates (runner-protocol, envoy-protocol, epoxy-protocol, ups-protocol, depot-protocol, data, depot) is consistently updated.
  • The rivet-util-serde to rivet-util dependency swap in envoy-protocol/Cargo.toml is correct; the macros live in rivet-util::serde which re-exports from rivet-util-serde plus adds the observability macros on top.
  • The compat_ack_tunnel_message change in pegboard-runner/src/ws_to_tunnel_task.rs correctly uses a type annotation for inference rather than a turbofish on the macro.
  • The test fixtures in envoy-protocol/src/versioned/mod.rs are updated consistently with the production paths.
  • The gasoline StateGuard change from observe!(serde_json::from_str::<T>(guard.0.get())?)? to json_from_str!(guard.0.get())? is semantically equivalent: the ? moved outside the macro but json_from_str! already wraps with observe! internally, so timing and size recording are preserved.
  • No new unbounded metric labels are introduced. The location label used by SERIALIZE_SIZE / DESERIALIZE_SIZE is compile-time file/line/col, so it is bounded by the number of call sites in the binary rather than runtime data.
  • No protocol schema changes — this is a pure call-site migration with no wire-format impact.

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