[signal, management, client, cmd] Create shared pprof package and allow pprof port configuration through NB_PPROF_ADDR#4971
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors pprof handling: removes ad-hoc HTTP pprof servers from service entry points, centralizes optional pprof initialization in a new util/pprof package (build-tag gated), adds blank imports to enable the package, and changes the default pprof address to localhost:6060. Changes
Sequence Diagram(s)(Skipped — changes are primarily build-tag, import and packaging adjustments rather than new multi-component runtime control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable sequence diagrams in the walkthrough.Disable the |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
management/cmd/pprof.go (1)
29-29: Remove trailing\nfrom log format string.Logrus automatically appends a newline; the explicit
\nis unnecessary.🔎 Apply this diff:
- log.Infof("listening pprof on: %s\n", listenAddr) + log.Infof("listening pprof on: %s", listenAddr)signal/cmd/pprof.go (2)
29-29: Remove trailing\nfrom log format string.Logrus automatically appends a newline; the explicit
\nis unnecessary.🔎 Apply this diff:
- log.Infof("listening pprof on: %s\n", listenAddr) + log.Infof("listening pprof on: %s", listenAddr)
1-33: Note: Intentional code duplication between signal and management pprof implementations.This file is structurally identical to
management/cmd/pprof.go. While this duplication is intentional (each package maintains its own pprof integration), consider whether a shared helper in a common package would reduce maintenance burden if the pprof pattern needs to evolve in the future.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
management/cmd/pprof.go(1 hunks)management/main.go(0 hunks)signal/cmd/pprof.go(1 hunks)signal/cmd/run.go(1 hunks)
💤 Files with no reviewable changes (1)
- management/main.go
🔇 Additional comments (4)
signal/cmd/run.go (1)
11-11: LGTM - Clean removal of hardcoded pprof integration.The removal of the
net/http/pprofimport and associated pprof startup logic is correct. Pprof functionality is now properly isolated in the build-taggedsignal/cmd/pprof.gofile.management/cmd/pprof.go (2)
1-2: LGTM - Correct build tag syntax.Using both the modern
//go:builddirective and legacy// +buildensures compatibility across Go versions.
14-17: Consider error handling for pprof server startup in goroutine frominit().The current pattern of using
log.Fatalfin a goroutine started frominit()is problematic: log.Fatalf is semantically equivalent to a non-recoverable panic without a stack-trace that prevents any deferred functions from running. This means:
- If the pprof server fails to bind (e.g., port already in use), the goroutine crashes the entire application after main has started, without allowing cleanup
- Multiple servers built with
-tags pprofon the same host without configuringNB_PPROF_ADDRcreates a port conflict scenarioUse
log.Errorfinstead, allowing graceful degradation, or implement synchronous port availability validation during startup with proper error signaling.signal/cmd/pprof.go (1)
1-2: LGTM - Correct build tag syntax.Using both the modern
//go:builddirective and legacy// +buildensures compatibility across Go versions.
| go pprof(addr) | ||
| } | ||
|
|
||
| func pprofAddr() string { |
There was a problem hiding this comment.
I think it will be better to centralize the duplicated functions into a shared package
|
Oh, i also encountered that... |
ujaandas
changed the title
|
Sorry for the delay! I've edited the PR title to reflect the new changes. I've just made a shared package inside |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@util/pprof/pprof_off.go`:
- Around line 5-8: Update the explanatory block comment in
util/pprof/pprof_off.go by correcting the misspelled word "exlude" to "exclude"
so the comment reads: "Otherwise, Go complains that 'build constraints exclude
all Go files'"; keep the rest of the comment intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 646bd985-5ac7-48f7-8972-309188dd96e9
📒 Files selected for processing (8)
client/cmd/root.gomanagement/cmd/root.gorelay/cmd/pprof.gorelay/cmd/root.gosignal/cmd/root.gosignal/main.goutil/pprof/pprof.goutil/pprof/pprof_off.go
💤 Files with no reviewable changes (1)
- relay/cmd/pprof.go
✅ Files skipped from review due to trivial changes (2)
- signal/cmd/root.go
- signal/main.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
4 participants
Describe your changes
Signal and Management servers previously hard‑coded a pprof listener on
localhost:6060, which caused port conflicts when multiple services ran in the same namespace (ie; in the same pod or as a NixOS service). This PR refactors both to use the samepprof.gohelper pattern already present in Client/Relay.Does this change build semantics though? To test it, I ran it with
-tags pprof. If it does, it shouldn't be a huge change to just slap whatpprof.godoes directly into the relevant functions.Issue ticket number and link
Fixes #4923
Stack
Branch:
pprof-configurable-signal-mgmtChecklist
Documentation
Select exactly one:
The behavior is consistent with existing Client/Relay profiling logic, so no new user‑facing configuration is introduced beyond the existing NB_PPROF_ADDR.
Summary by CodeRabbit
Refactor
Chores