You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
PR Review: feat(rivetkit): gcp log level format for rivet compute
This PR adds GCP Cloud Logging severity mapping for pino when running on Rivet Compute (_RIVET_COMPUTE_ENABLED=1). The approach is clean and the scope is well-contained. A few items to address:
Issues
Silent formatter override in createGcpLoggingPinoConfig
In gcp-log-helper.ts, if the caller passes formatters.level inside pinoLoggerOptionsMixin, it gets silently overwritten:
This is probably intentional since GCP severity mapping is the whole point of this function, but a comment would prevent future confusion. Consider:
```ts
// Always override level formatter; GCP severity mapping is the purpose of this config.
level: pinoLevelToGcpSeverity,
```
typeof window === 'undefined' guard is fragile
The check in log.ts uses typeof window === 'undefined' to detect server-side execution. This is a common pattern but can produce unexpected results in some edge runtimes (e.g., Cloudflare Workers, Deno) where window may or may not exist. Since this is gated behind _RIVET_COMPUTE_ENABLED (a Rivet-internal env var that only exists in Rivet's own compute environment), this is probably fine in practice. Worth a brief comment explaining why the window check is there alongside the compute check.
Minor Notes
No tests added.pinoLevelToGcpSeverity is a pure function with a fixed lookup table, making it trivial to unit-test. At minimum a short test covering the known mappings and the unknown-level fallback would increase confidence. The behavior change in configureDefaultLogger could be covered by checking that baseLogger uses the GCP formatter when the env var is set.
GCP severity mapping looks correct.trace → DEBUG, fatal → CRITICAL, unknown → INFO are all the right choices per GCP's severity ladder.
PR description is empty. The default template was left entirely blank. A one-line summary of the motivation (e.g., "structured GCP log entries require the severity field instead of the pino level field") would help reviewers and future git log readers.
Summary
The implementation is straightforward and correct. The main asks before merging:
Add a comment on the silent formatter override in createGcpLoggingPinoConfig.
Fill in the PR description.
(Optional but recommended) Add unit tests for pinoLevelToGcpSeverity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: