Add support for profiling via pprof #593
Conversation
Disabled by default in case we're incurring a performance hit
WalkthroughThe changes enhance the application's profiling capabilities. In Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment
participant F as Flags Init
participant M as Main
participant CM as Manager (ctrl.NewManager)
Env->>F: Read ENABLE_PROFILING variable
F-->>M: Return EnableProfiling (true/false)
M->>M: Set pprofBindAddr = ":8281" if profiling enabled
alt Profiling Enabled
M->>CM: Initialize Manager with PprofBindAddress = ":8281"
else
M->>CM: Initialize Manager without PprofBindAddress
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
![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 (2)
pkg/flags/flags.go (1)
17-17: LGTM! Consider adding field-level documentation.The new
EnableProfilingfield follows the existing patterns and conventions. Consider adding a field-level comment to document its purpose and the environment variable that controls it.type Features struct { DisablePodTopologySpreadConstraints bool + // EnableProfiling enables runtime profiling via pprof when set to true. + // Controlled by SKIPERATOR_ENABLE_PROFILING environment variable. EnableProfiling bool }cmd/skiperator/main.go (1)
73-76: Document the hardcoded port number.The pprof port (8281) should be documented to explain why this specific port was chosen and to make it easier to find and update if needed.
- pprofBindAddr := "" - if flags.FeatureFlags.EnableProfiling { - pprofBindAddr = ":8281" - } + // Port 8281 is reserved for pprof profiling to avoid conflicts with other ports: + // - 8081: Health probe + // - 8181: Metrics + pprofBindAddr := "" + if flags.FeatureFlags.EnableProfiling { + pprofBindAddr = ":8281" + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/skiperator/main.go(2 hunks)pkg/flags/flags.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and run tests
- GitHub Check: Build and run tests
🔇 Additional comments (2)
pkg/flags/flags.go (1)
23-23: LGTM! The initialization follows existing patterns.The field is properly initialized using the same helper function and follows the environment variable naming convention.
cmd/skiperator/main.go (1)
85-85: LGTM! The pprof configuration is properly integrated.The pprof binding address is correctly passed to the manager configuration.
|
That's correct @omaen. It is only accessible if we port-forward to that port or expose it via a |
Disabled by default in case we're incurring a performance hit
Summary by CodeRabbit