Adding a before-call-begins fxevent, to help with troubleshooting #1267
Conversation
WalkthroughThe changes introduce a new event type called "BeforeRun" and update the associated logging and test validations. Test files now assert the presence of "BeforeRun" events before "Run" events. The event package defines the new "BeforeRun" type with its corresponding method, while various logger implementations (console, slog, and zap) and module methods are updated to log "BeforeRun" events. There are no modifications to exported or public entities. Changes
Sequence Diagram(s)sequenceDiagram
participant M as "Module Operation"
participant CB as "BeforeRun Callback"
participant L as "Logger (Console/Slog/Zap)"
M->>CB: Invoke operation (provide/supply/decorate/replace)
CB->>L: Log BeforeRun event (name, kind, module)
L-->>CB: Acknowledge log
CB-->>M: Callback complete, proceed with operation
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Groxx
changed the title
Groxx
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go.mod (1)
19-22: Temporary Dependency Replacement for DigThe addition of the replace directive to point
go.uber.org/digtogithub.com/Groxx/digat versionv1.18.2-0.20250324200048-72ef97acadd5is correctly implemented as a temporary fix. The TODO comment reminds us to remove this change once the upstream "before-callback" PR is integrated. Make sure to update or remove this directive promptly when the upstream changes are merged to avoid future conflicts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod(1 hunks)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1267 +/- ##
==========================================
+ Coverage 98.62% 98.64% +0.01%
==========================================
Files 35 35
Lines 3411 3458 +47
==========================================
+ Hits 3364 3411 +47
Misses 40 40
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JacobOaks
left a comment
JacobOaks
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the PR! This should definitely help w/ some tricky debugging situations.
2 participants
What
Adds a "BeforeRun" fxevent.Event, mirroring "Run" but it is called before the provided func is called by Dig, rather than after.
This relies on uber-go/dig#431 (released as v1.19.0).
Motivation
A recent startup-issue debugging session included logs like this:
which accurately reflected the logs the app produced: during startup, it was getting OOM-killed after the final log, several seconds after the last Fx-"run" event.
Unfortunately, Fx's
Runevent is logged after a call completes, so the fxevent log immediately before the application-produced logs tells us almost nothing about what is currently running. That seems... kinda not ideal. I've run into this quite a few times over the years, but I finally decided to do something about it.So I added a "before run" event, so this log will be much easier to diagnose:
After a quick attempt in Fx it became pretty clear that it would be difficult or almost impossible to do without changing Dig, so this PR relies on a change to Dig: uber-go/dig#431
Summary by CodeRabbit
New Features
Tests
Chores