Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Jan 6, 2026

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

Project Deployment Review Updated (UTC)
rivet-cloud Ready Ready Preview, Comment Jan 6, 2026 11:48pm
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-inspector Ignored Ignored Preview Jan 6, 2026 11:48pm
rivet-site Ignored Ignored Preview Jan 6, 2026 11:48pm
rivetkit-serverless Skipped Skipped Jan 6, 2026 11:48pm

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless January 6, 2026 23:46 Inactive
Copy link
Contributor Author

MasterPtato commented Jan 6, 2026


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 Jan 6, 2026

PR Review: Fix dashboards, add pb metrics, fix listen_n order

I've reviewed this PR and found it to be a solid maintenance update with several important fixes. Here are my findings:

Positive Aspects

1. Critical Bug Fix - Signal Ordering (gasoline/src/db/kv/mod.rs:2129)

  • Added signals.sort_by_key to ensure signals are returned in timestamp order
  • This is crucial for correctness when multiple signals of different types are received
  • The comment correctly notes signals are already ordered within individual streams, making this sort operation cheap
  • Good defensive programming practice

2. Metrics Addition (pegboard/src/metrics.rs)

  • Added RUNNER_VERSION_UPGRADE_DRAIN_TOTAL metric to track version upgrade drains
  • Added SERVERLESS_OUTBOUND_REQ_TOTAL metric to track serverless outbound requests
  • Both metrics include appropriate labels for proper observability

3. Dashboard Improvements

  • Updated Grafana dashboards with better metric queries and visualizations
  • Fixed timestamp calculation in gasoline dashboard (converting ms to seconds properly)
  • Reorganized guard dashboard with clearer sections (TCP, Proxy Requests, Cache)
  • Added error rate panel for better monitoring

4. API Schema Update

  • Moved drain_on_version_upgrade field to top level in OpenAPI spec

Issues Found

CRITICAL: Duplicate Metric Name (pegboard/src/metrics.rs:26-31)

The metric name is a copy-paste error - SERVERLESS_OUTBOUND_REQ_TOTAL uses "pegboard_runner_version_upgrade_drain_total" as its metric name, which is already used by RUNNER_VERSION_UPGRADE_DRAIN_TOTAL on line 19. This will either:

  • Cause a panic at runtime due to duplicate metric registration, or
  • Cause data corruption by mixing two different metrics under the same name

This must be fixed before merging. Change line 27 to use "pegboard_serverless_outbound_req_total" instead.

Minor Observations

1. Serverless Connection Code (pegboard/src/workflows/serverless/conn.rs:401-403)

  • Metric increment happens before the actual request completes
  • Current behavior counts attempts, not successes - ensure this aligns with monitoring goals

2. Dashboard Consistency

  • All 5 docker environment variants are updated consistently
  • Template file properly updated

Security & Performance

  • No security concerns identified
  • Sort operation is efficient as noted in comments
  • Proper error handling maintained throughout
  • No new dependencies added

Testing

  • The changes affect monitoring and observability primarily
  • Signal ordering fix should be tested with workflows that receive multiple signal types concurrently
  • Recommend manual verification that the new metrics appear in Prometheus/Grafana after deployment

Verdict

MUST FIX BEFORE MERGE: The duplicate metric name in pegboard/src/metrics.rs line 27.

Once fixed, this PR will be ready to merge. The signal ordering fix is important for correctness, and the observability improvements are valuable additions.

@MasterPtato MasterPtato force-pushed the 01-06-fix_fix_dashboards_add_pb_metrics_fix_listen_n_order branch from 63497dc to ed662df Compare January 7, 2026 20:29
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 7, 2026

Merge activity

  • Jan 7, 8:56 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 7, 8:57 PM UTC: CI is running for this pull request on a draft pull request (#3779) due to your merge queue CI optimization settings.
  • Jan 7, 8:58 PM UTC: Merged by the Graphite merge queue via draft PR: #3779.

@graphite-app graphite-app bot closed this Jan 7, 2026
@graphite-app graphite-app bot deleted the 01-06-fix_fix_dashboards_add_pb_metrics_fix_listen_n_order branch January 7, 2026 20:58
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.

3 participants