Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Dec 17, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

4 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Dec 17, 2025 9:41pm
rivet-inspector Ignored Ignored Preview Dec 17, 2025 9:41pm
rivet-site Ignored Ignored Preview Dec 17, 2025 9:41pm
rivetkit-serverless Skipped Skipped Dec 17, 2025 9:41pm

Copy link
Contributor Author

MasterPtato commented Dec 17, 2025

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.

@claude
Copy link

claude bot commented Dec 17, 2025

Code Review - PR #3657: fix: add wf and guard metrics

I've reviewed this PR which adds and improves metrics for workflows (gasoline) and guard components. Overall, the changes are solid and improve observability. Here are my findings:

✅ Positive Aspects

  1. Metric Naming Convention Fix: The rename from api_request_errors to api_request_errors_total in api-builder/src/metrics.rs:27 follows Prometheus best practices for counter metrics (counters should have _total suffix).

  2. New Guard Metrics: Adding IN_FLIGHT_REQUEST_COUNT metric provides valuable observability into concurrent request handling.

  3. Improved Loop Metrics: The refactored loop iteration timing now measures the actual iteration duration (including the callback) rather than just overhead, which is more useful for performance analysis.

🔍 Code Quality & Best Practices

Gasoline Loop Metrics (lupe.rs:168-193)

  • Good: The refactoring correctly captures the callback duration (cb_dt) and only records it for non-replay iterations
  • Good: Parallel execution is maintained with tokio::join! for optimal performance
  • Minor: The iteration_dt is captured from the previous iteration and recorded in the current iteration's branch commit block. This works correctly but could be confusing. Consider adding a comment explaining this deferred recording pattern.

Guard In-Flight Tracking (proxy_service.rs:628-648)

  • Potential Issue: The generate_request_id function has a subtle race condition pattern:
    self.in_flight_requests
        .entry(request_id)
        .or_insert_with(async {
            inserted = true;
        })
        .await;
    The inserted flag pattern works, but the metric update happens outside the atomic operation. If multiple threads generate the same ID simultaneously, the metric could be slightly inaccurate. However, given the UUID collision probability is astronomically low, this is acceptable.

🐛 Potential Bugs

None identified - The code appears functionally correct.

⚡ Performance Considerations

  1. Loop Metric Recording: The metrics are now recorded only for non-replay iterations (line 189-192), which is correct and avoids polluting metrics with replay data.

  2. Cache Entry Count: Using entry_count() on moka::Cache (lines 621, 642) is efficient - it's an O(1) operation.

  3. Removed Dead Code: Excellent cleanup removing 90 lines of unused StructuredResponse and related code in proxy_service.rs. This was dead code that could never be reached.

🔒 Security Concerns

None identified - No security-related changes in this PR.

🧪 Test Coverage

Recommendation: Consider adding tests for:

  1. Verify that LOOP_ITERATION_DURATION correctly measures the callback execution time
  2. Verify that IN_FLIGHT_REQUEST_COUNT accurately tracks concurrent requests
  3. Verify that loop metrics are NOT recorded during replay iterations

The PR doesn't include test changes, but given these are metric additions rather than logic changes, the risk is low.

📝 Additional Observations

  1. Metric Description Update (gasoline/metrics.rs:183): The description changed from "excluding its body" to "Total duration of a single loop iteration" which now accurately reflects what's being measured.

  2. Code Cleanup: The removal of the entire StructuredResponse infrastructure (lines 137-145, 402-414, etc. in proxy_service.rs) suggests this was an unused feature. Good cleanup! ✨

  3. Logging Guideline Adherence: The structured logging follows the CLAUDE.md guidelines (lowercase messages, proper use of tracing spans).

Recommendation: APPROVE

This PR improves observability and cleans up dead code. The metric changes are semantically correct and follow best practices. No blocking issues identified.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 17, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: e258ce3

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