Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughStart no longer fails for usage-tracker initialization errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2489 +/- ##
==========================================
+ Coverage 56.05% 61.72% +5.67%
==========================================
Files 215 231 +16
Lines 23548 24154 +606
==========================================
+ Hits 13199 14909 +1710
+ Misses 9107 8002 -1105
- Partials 1242 1243 +1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/router.go (1)
1400-1424:⚠️ Potential issue | 🟠 MajorPotential nil-pointer panic in Shutdown if usage tracking fails.
If
track.NewUsageTrackerreturns an error at Line 1418,configureUsageTrackingreturns the error at Line 1420. The caller (Startat Line 1206-1208) logs the error but continues. However,r.usageremainsnilbecause it was never assigned after the failure. Later,Shutdowncallsr.usage.Close()at Line 1577, which will panic on a nil receiver.Consider setting
r.usageto a no-op tracker when initialization fails:🐛 Proposed fix
r.usage, err = track.NewUsageTracker(r.logger, cfg) if err != nil { + r.usage = &UsageTrackerNoOp{} return err }
🤖 Fix all issues with AI agents
In `@router/internal/track/track.go`:
- Around line 56-63: The PostHog endpoint string in the posthog.Config is
misspelled ("https://eu.i.posthoga.com"); update the Endpoint value used when
creating cfg (the variable used with posthog.NewWithConfig) to the correct EU
Cloud URL "https://eu.i.posthog.com" so tracker.client is initialized against
the right host (refer to tracker.posthogLogger(), cfg, and posthog.NewWithConfig
in the file to locate the change).
…errors-shouldnt-be-logged-as-error
…ldnt-be-logged-as-error' into ale/eng-8779-posthog-errors-shouldnt-be-logged-as-error
@coderabbitai But Close isn't already checking about nil receiver? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 1068 🏁 Script executed: Length of output: 1019
Looking at the func (u *UsageTracker) Close() {
if u == nil || u.client == nil {
return
}
_ = u.trackRouterUptime(uptimeOptions{closed: true})
_ = u.client.Close()
}The method safely handles the case where Thank you for catching this! 🙏 🧠 Learnings used |
…errors-shouldnt-be-logged-as-error
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/core/router.go (1)
1397-1418:⚠️ Potential issue | 🟡 MinorAlign tracker-init failure log level with the stated Info-level requirement.
The PR objectives mention logging an Info message when PostHog is unavailable at start. Right now
track.NewUsageTrackerfailures are logged at Debug, which can hide the “tracking disabled” signal in production. Consider upgrading this specific log to Info (or adding a targeted Info log for PostHog unavailability).Proposed adjustment
- r.logger.Debug("failed to start usage tracking", zap.Error(err)) + r.logger.Info("usage tracking disabled; failed to start usage tracker", zap.Error(err))
PostHog is considered important, when really it is not. With this PR I'm changing this so that:
Summary by CodeRabbit
Bug Fixes
Refactor
Checklist