[signal, management] Allow pprof port configuration through NB_PPROF_ADDR

[ujaandas]

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 same pprof.go helper 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 what pprof.go does directly into the relevant functions.

Issue ticket number and link

Fixes #4923

Stack

Branch: pprof-configurable-signal-mgmt

Checklist

• Is it a bug fix
• Is a typo/documentation fix
• Is a feature enhancement
• It is a refactor
• Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

• I added/updated documentation for this change
• Documentation is not needed for this change (explain why)

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
  • Reorganized profiling infrastructure for optional runtime profiling support through build configuration.
  • Profiling address now configurable via NB_PPROF_ADDR environment variable (defaults to localhost:6060).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[CLAassistant]

All committers have signed the CLA.

[coderabbitai]

Walkthrough

The pull request refactors pprof profiling support from hardcoded runtime initialization to an optional, configurable mechanism. Pprof integration is removed from main entry points and relocated to separate build-tagged files that read NB_PPROF_ADDR environment variable for address configuration, enabling both management and signal servers to run on the same host without port conflicts.

Changes

Cohort / File(s)	Summary
Removal of hardcoded pprof management/main.go, signal/cmd/run.go	Deleted imports and initialization code that unconditionally started pprof HTTP server on startup.
Optional build-tagged pprof support management/cmd/pprof.go, signal/cmd/pprof.go	Added new build-tagged modules (guarded by pprof tag) that register init functions to start pprof server. Address is read from NB_PPROF_ADDR environment variable with default localhost:6060. Includes helper functions for address resolution and server startup with error logging.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Verify NB_PPROF_ADDR environment variable name is consistently used across both files
• Confirm default address (localhost:6060) is identical in both implementations
• Ensure build tag syntax and init function execution flow are correct
• Validate that removals from main files don't leave orphaned references elsewhere

Poem

🐰 Two servers clashed on port six-oh-six,
Now flags and vars help us fix,
Build with pprof when you dare,
Set addresses with care,
No more conflicts—just metrics to mix! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main change: making pprof port configurable through the NB_PPROF_ADDR environment variable across signal and management services.
Description check	✅ Passed	The description adequately explains the problem (hardcoded pprof port conflicts), the solution (using NB_PPROF_ADDR like Client/Relay), and references the fixed issue #4923 with appropriate checklist completion.
Linked Issues check	✅ Passed	The PR fully addresses issue #4923 by making pprof port configurable via NB_PPROF_ADDR environment variable for both Signal and Management services, eliminating port conflicts when running multiple services on the same host.
Out of Scope Changes check	✅ Passed	All changes are directly related to the objective: refactoring Signal and Management pprof initialization to use configurable NB_PPROF_ADDR. No unrelated modifications detected.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

management/cmd/pprof.go (1)

29-29: Remove trailing \n from log format string.

Logrus automatically appends a newline; the explicit \n is 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 \n from log format string.

Logrus automatically appends a newline; the explicit \n is 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

📥 Commits

Reviewing files that changed from the base of the PR and between 537151e and 6e4d355.

📒 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/pprof import and associated pprof startup logic is correct. Pprof functionality is now properly isolated in the build-tagged signal/cmd/pprof.go file.

management/cmd/pprof.go (2)

1-2: LGTM - Correct build tag syntax.

Using both the modern //go:build directive and legacy // +build ensures compatibility across Go versions.

---

14-17: Consider error handling for pprof server startup in goroutine from init().

The current pattern of using log.Fatalf in a goroutine started from init() 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 pprof on the same host without configuring NB_PPROF_ADDR creates a port conflict scenario

Use log.Errorf instead, 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:build directive and legacy // +build ensures compatibility across Go versions.