Skip to content

feat(rivetkit): gcp log level format for rivet compute#4542

Draft
abcxff wants to merge 1 commit intomainfrom
03-31-feat_rivetkit_gcp_log_level_format_for_rivet_compute
Draft

feat(rivetkit): gcp log level format for rivet compute#4542
abcxff wants to merge 1 commit intomainfrom
03-31-feat_rivetkit_gcp_log_level_format_for_rivet_compute

Conversation

@abcxff
Copy link
Copy Markdown
Contributor

@abcxff abcxff commented Mar 31, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link
Copy Markdown

railway-app bot commented Mar 31, 2026

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

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Mar 31, 2026 at 9:29 pm
frontend-inspector 😴 Sleeping (View Logs) Web Mar 31, 2026 at 9:25 pm
frontend-cloud 😴 Sleeping (View Logs) Web Mar 31, 2026 at 9:24 pm
mcp-hub ✅ Success (View Logs) Web Mar 31, 2026 at 9:16 pm
ladle ❌ Build Failed (View Logs) Web Mar 31, 2026 at 9:16 pm
kitchen-sink ❌ Build Failed (View Logs) Web Mar 31, 2026 at 9:15 pm

Copy link
Copy Markdown
Contributor Author

abcxff commented Mar 31, 2026

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

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Mar 31, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/sqlite-vfs

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sqlite-vfs@4542

@rivetkit/traces

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

@rivetkit/workflow-engine

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/workflow-engine@4542

@rivetkit/virtual-websocket

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 8d0f41e

@claude
Copy link
Copy Markdown

claude bot commented Mar 31, 2026

PR Review: feat(rivetkit): gcp log level format for rivet compute

Summary: This PR adds GCP Cloud Logging severity mapping for pino when running on Rivet Compute, controlled by a new internal env var _RIVET_COMPUTE_ENABLED. The change is small (~60 lines) and the core idea is sound, but there are several issues worth addressing before merging.


What's Good

  • Inlining the GCP severity mapping instead of pulling in a full dependency is the right call. Less bloat, no extra dep to maintain.
  • The PINO_TO_GCP_SEVERITY mapping is correct per GCP severity levels: trace/debug -> DEBUG, info -> INFO, warn -> WARNING, error -> ERROR, fatal -> CRITICAL.
  • The typeof window === "undefined" guard correctly restricts the GCP path to server-side Node.js environments.

Issues

1. env-vars.ts comment explicitly requires updating the docs, but the PR does not

env-vars.ts has a project-enforced comment:

// IMPORTANT: When adding or modifying environment variables here, also update the
// documentation at: website/src/content/docs/general/environment-variables.mdx

_RIVET_COMPUTE_ENABLED does not appear in website/src/content/docs/general/environment-variables.mdx. Even if the intent is to leave this undocumented publicly, the inline comment "Intentionally undocumented for now" directly contradicts the IMPORTANT instruction at the top of the file. Either add a minimal internal note to the doc or get explicit sign-off that this env var is intentionally omitted.

2. Redundant level field in GCP output

In gcp-log-helper.ts, the return value is { severity, level: pinoSeverityLevel }. GCP Cloud Logging already receives the severity in the severity field. Including the numeric level is redundant noise in the JSON payload. Consider returning only { severity }, or add a comment explaining why both fields are intentionally included.

3. GCP logger path silently drops browser write handlers and hooks.logMethod

The existing pino(...) call configures browser.write handlers and hooks.logMethod for custom logfmt-style output. The new GCP path creates a plain pino(createGcpLoggingPinoConfig(...)) without these. This is almost certainly intentional (GCP compute is server-side), but it means any code relying on the logMethod hook for field ordering or the custom customWrite format will behave differently when _RIVET_COMPUTE_ENABLED=1. This behavioral difference should be documented with a comment.

4. formattersMixin spread can silently clobber the level formatter for future callers

In gcp-log-helper.ts:

formatters: {
    ...formattersMixin,
    level: pinoLevelToGcpSeverity,
},

The level key is placed after ...formattersMixin, so the GCP formatter always wins. Correct for the current single caller. However, if pinoLoggerOptionsMixin ever includes a formatters.level, it will be silently overridden. Either document this override behavior explicitly or restructure to make the override unambiguous.

5. Non-standard parameter names in pinoLevelToGcpSeverity

Pino's formatters.level signature is (label: string, number: number): object. The PR uses pinoSeverityLabel / pinoSeverityLevel which are verbose and non-standard compared to pino's own naming. Using standard pino naming would make cross-referencing pino docs easier.

6. No tests added

No test files are included. At minimum, a unit test for pinoLevelToGcpSeverity covering known mappings and the unknown-label fallback to "INFO" would provide a safety net. The PR is in DRAFT state so this may be intentional, but it should be addressed before marking ready for review.


Minor / Style

  • The ternary in gcp-log-helper.ts is split across two lines in a way that is slightly inconsistent with the rest of the file. Not a blocker.
  • _RIVET_COMPUTE_ENABLED follows the internal env var naming convention (_RIVET_*) consistent with other internal vars in env-vars.ts. Good.

Summary

Area Status
Core logic (severity mapping) Correct
Dependency hygiene (no new dep) Good
Docs update required by inline comment Missing
Test coverage Missing
GCP path drops browser/hooks (intentional but undocumented) Needs comment
formattersMixin spread silently overrides caller level Needs doc/comment
Numeric level field redundant in GCP output Minor

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