Skip to content

[SLOP(claude-opus-4-8)] feat(rivetkit-core): add serde duration and size metrics#5322

Open
MasterPtato wants to merge 1 commit into
stack/add-observation-for-serde_bare-oskoutmwfrom
stack/slop-claude-opus-4-8-feat-rivetkit-core-add-serde-duration-and-size-metrics-nqkrytvr
Open

[SLOP(claude-opus-4-8)] feat(rivetkit-core): add serde duration and size metrics#5322
MasterPtato wants to merge 1 commit into
stack/add-observation-for-serde_bare-oskoutmwfrom
stack/slop-claude-opus-4-8-feat-rivetkit-core-add-serde-duration-and-size-metrics-nqkrytvr

Conversation

@MasterPtato

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 5322
Push local edits: forklift submit
Merge when ready: forklift merge 5322

@claude

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

This PR adds serialization/deserialization duration and byte-size metrics to rivetkit-core, wrapping hot paths in persist.rs and registry/http.rs via a new serde_metrics module. The approach (LazyLock collector struct, measure_serialize/measure_deserialize helpers) is consistent with how actor/metrics.rs works, and the crate::time::Instant alias ensures WASM compatibility.


serde_metrics.rs:153register_metric is already defined in actor/metrics.rs

actor/metrics.rs contains an identical generic helper (same bounds, same body) differing only in the log message. Two copies of this helper will drift over time. The helper should be extracted to a shared location (e.g. a metrics submodule or the existing actor/metrics.rs re-exported) and reused here.


serde_metrics.rs:25serde_size_buckets() should be a constant in engine/packages/metrics/src/buckets.rs, not a private fn

CLAUDE.md: "Add a new constant to buckets.rs only if no existing constant covers the value range." The byte-size range (16–16M bytes) has no existing constant there; MICRO_BUCKETS/BUCKETS/PAGE_COUNT_BUCKETS are all time-domain or page-count values. The fix is to add pub const BYTE_SIZE_BUCKETS: &[f64] to buckets.rs (same as this PR's values). As a private fn it can't be reused from other crates that add byte-size histograms later.


serde_metrics.rs:26 — Missing 512-byte bucket creates a 4× gap between 256 and 1024

The sequence jumps 256.0 → 1024.0. Every other adjacent step in the array is 2–4×, but this particular gap is 4× in a byte range where typical actor state and action payloads land. Adding 512.0 between those two entries brings the resolution in line with the rest of the buckets without changing anything else.


registry/http.rs:778encoding_format_label duplicates the HttpResponseEncoding-to-string match already in content_type_for_encoding

Two separate match arms over the same enum mean adding a new HttpResponseEncoding variant requires updating two free functions. While exhaustive matching catches the missing arm at compile time, the string used in metrics and the string used in content-type headers live in different places. An impl HttpResponseEncoding { fn format_label(&self) -> &'static str } method (or a single shared helper) would co-locate the knowledge.


serde_metrics.rs:91,116serialize_size and deserialize_size have asymmetric success semantics

serialize_size records bytes only on Ok (output bytes of successful encodes). deserialize_size records bytes unconditionally before the closure runs (input bytes for all attempts, including failures). Both histograms are named *_size and look symmetric on a dashboard, but a failed-decode retry will inflate deserialize_size without a corresponding serialize_size entry. The module doc comment mentions this for deserialize but does not explain why the two differ, making it easy for dashboard authors to treat them as equivalent when they are not. Consider either: (a) making deserialize only record on success too, or (b) adding a result label (ok/err) so the asymmetry becomes explicit and queryable.

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