Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@railway-app
Copy link

railway-app bot commented Jan 16, 2026

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

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Jan 16, 2026 at 9:53 am
frontend-inspector 😴 Sleeping (View Logs) Web Jan 16, 2026 at 9:38 am
frontend-cloud 😴 Sleeping (View Logs) Web Jan 16, 2026 at 9:34 am

Copy link
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 16, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3928

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3928

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3928

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3928

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3928

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3928

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3928

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3928

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3928

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3928

commit: 88935a1

@claude
Copy link

claude bot commented Jan 16, 2026

PR Review: Add Actor and KV Metrics

Thank you for this contribution! This PR adds comprehensive metrics tracking for Rivet Actors and KV operations. Here's my detailed review:


🐛 Critical Issues

1. Compilation Error in delete function (engine/packages/pegboard/src/actor_kv/mod.rs:331-361)

The delete function references recipient which doesn't exist in the function scope - lines 341-345 reference recipient.namespace_id and recipient.name but the function signature only has actor_id.

Fix: The delete function needs to accept a Recipient parameter like get, list, and put do, and update all call sites in pegboard-runner.

2. Type Mismatch in MetricKey deserialization (engine/packages/pegboard/src/keys/ns/metric.rs:85)

The deserialize function returns usize when the Value type is i64. Should use i64::from_le_bytes instead of usize::from_le_bytes.

3. Compilation Error in get_for_runner (engine/packages/pegboard/src/ops/actor/get_for_runner.rs:29-46)

References undefined variable actor_id (should be input.actor_id) and returns Actor type instead of Output.


⚠️ Design & Best Practices Issues

4. Missing metrics in delete_all (engine/packages/pegboard/src/actor_kv/mod.rs:365)

The delete_all function doesn't track KV write metrics while delete does. Either add metrics tracking or document why it's excluded.

5. Inconsistent metric tracking in clear_kv (engine/packages/pegboard/src/workflows/actor/destroy.rs:180-199)

Gets the final KV size but doesn't update metrics. Consider adding a metric decrement for storage freed.

6. Module organization (engine/packages/pegboard/src/actor_kv/entry.rs)

Imports reference types from keys/actor_kv.rs not entry.rs. The module structure could be clearer.

7. Repetitive unpacking code (engine/packages/pegboard/src/keys/ns/metric.rs:138-251)

Significant code duplication in TupleUnpack implementation. Consider using a macro to reduce repetition.

8. Integer overflow handling (engine/packages/pegboard/src/actor_kv/mod.rs:133)

Using unwrap_or_default means overflow becomes 0. Consider unwrap_or(i64::MAX) to saturate instead.


✅ Positive Aspects

  1. Good refactoring: Moving actor KV into pegboard simplifies architecture
  2. Comprehensive metrics: Tracks all important dimensions
  3. Proper structured logging: Uses tracing correctly
  4. Transaction safety: Metric updates properly integrated

🔍 Test Coverage

Test files are empty placeholders. Add tests for metric increments, edge cases, and overflow scenarios.


📋 Summary

Must Fix Before Merge:

  1. Fix delete function to accept Recipient parameter
  2. Fix MetricKey::deserialize type mismatch
  3. Fix get_for_runner compilation errors

Should Address:
4. Add metrics to delete_all
5. Track freed storage in clear_kv
6. Add test coverage

Overall a valuable addition for observability! Once compilation issues are fixed this will be ready to merge.

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.

2 participants