Add pprof to ledger service - #8631
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughThe ledger service now initializes an auto-profiler, exposes runtime profiler settings through admin commands, restricts pprof access to loopback requests, and documents profiling operations. ChangesLedger profiler
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@admin/README.md`:
- Around line 94-105: Update the “Ledger service profiler” section in
admin/README.md to state that the ledger admin server is disabled by default
because --admin-addr defaults to an empty value, and add the required startup
example using --admin-addr=127.0.0.1:9003 before the curl commands.
In `@cmd/ledger/admin.go`:
- Around line 51-56: Update newAdminHandler so pprof handlers are not exposed
through the publicly bindable admin mux; either restrict the admin listener to
loopback or serve pprof through a separate authenticated/authorized listener,
while preserving the existing admin endpoints.
In `@cmd/ledger/main.go`:
- Line 304: Configure the shared adminServer HTTP server with ReadHeaderTimeout,
ReadTimeout, and IdleTimeout to bound slow admin and pprof connections, while
setting a sufficiently long WriteTimeout for intentional CPU/profile captures.
Update the server initialization associated with newAdminHandler without
changing the handler registrations.
- Around line 111-116: Synchronize access to currentBlockRate across the getter
and setter registered in RegisterUintConfig, protecting both dereference and
pointer replacement with the same mutex. Keep runtime.SetBlockProfileRate in the
setter and ensure concurrent get-config and set-config requests are race-free.
- Around line 75-80: Validate *profilerInterval before constructing
profiler.ProfilerConfig or calling profiler.New, rejecting values less than or
equal to zero through the command’s existing handled-error path; only create
autoProfiler after this validation succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4bd7f2e6-c918-4b56-9ae0-e7d66dbcaa1e
📒 Files selected for processing (3)
admin/README.mdcmd/ledger/admin.gocmd/ledger/main.go
| return | ||
| } | ||
| oldValue := field.Get() | ||
| if err := field.Set(configValue); err != nil { |
There was a problem hiding this comment.
Field.Set can fail with non-validation errors, e.g. TriggerRun returns "profiling is already in progress", which is server state, not a bad request. Check updatable_configs.IsValidationError(err) and return 400 only for validation errors, 500 otherwise, matching admin/commands/common/set_config.go.
| profilerDir = flag.String("profiler-dir", "profiler", "Directory to create auto-profiler profiles") | ||
| profilerInterval = flag.Duration("profiler-interval", 15*time.Minute, "Interval between auto-profiler runs") | ||
| profilerDuration = flag.Duration("profiler-duration", 10*time.Second, "Duration of each auto-profiler run") | ||
| profileUploaderEnabled = flag.Bool("profile-uploader-enabled", false, "Whether to upload profiles to a remote uploader (disabled for ledger service)") |
There was a problem hiding this comment.
nit: this flag's only effect is a warning that it is ignored.
|
|
||
| // Initialize updatable config manager and auto-profiler. | ||
| // The profiler is configured via admin get-config/set-config commands. | ||
| configManager := updatable_configs.NewManager() |
There was a problem hiding this comment.
nit: profiler.New creates the profile directory and starts a goroutine before the required-address validation below can fatal. Move this block after that validation so misconfigured invocations have no side effects.
| go func() { | ||
| <-autoProfiler.Ready() | ||
| logger.Info().Bool("enabled", autoProfiler.Enabled()).Msg("auto-profiler ready") | ||
| }() |
There was a problem hiding this comment.
nit: autoProfiler.Done() is never awaited at shutdown, so an in-flight profile run is truncated at exit. The goroutine is also unnecessary: with no readiness checks, Ready() closes almost immediately.
| ``` | ||
|
|
||
| ### Ledger service profiler | ||
| The standalone ledger service (`cmd/ledger`) exposes the same profiler configuration through its admin server (default port `9003`). It also registers the `net/http/pprof` endpoints on the admin server for on-demand profiling. |
There was a problem hiding this comment.
nit: cmd/ledger/README.md documents the admin commands and flags but was not updated with the new profiler flags and the get-config/set-config commands.
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)
cmd/ledger/admin.go (1)
129-162: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winProtect profiler configuration commands from remote callers.
The README shows
--admin-addrbound to0.0.0.0, while/admin/run_commandhas no access control. A remote client can invokeset-configto enable profiling, trigger captures, or increase runtime profiling overhead. Restrictget-config/set-configto loopback or require authenticated admin access.As per coding guidelines, “treat all inputs as potentially byzantine.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ledger/admin.go` around lines 129 - 162, Restrict the get-config and set-config branches in the admin command handler, including the set-config flow around h.configManager.GetField, to loopback callers or authenticated admin requests before reading or mutating configuration. Reject unauthorized remote requests with the existing HTTP error mechanism, while preserving access for trusted local or authenticated admin clients.Source: Coding guidelines
🧹 Nitpick comments (1)
cmd/ledger/admin.go (1)
40-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd regression coverage for the new admin boundary and config paths.
Patch coverage is 0% for this file. Add table-driven tests for loopback rejection/acceptance and
get-config/set-configsuccess, validation-error, and internal-error responses.Also applies to: 116-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ledger/admin.go` around lines 40 - 57, Add table-driven regression tests covering requireLoopback rejection of non-loopback and malformed RemoteAddr values, acceptance of loopback requests, and the get-config/set-config handlers’ success, validation-error, and internal-error responses. Exercise the existing handler and configuration symbols directly, asserting HTTP status codes and relevant response bodies so the new admin boundary and config paths are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ledger/main.go`:
- Around line 384-386: Update the auto-profiler startup in New to launch
runForever through p.unit.Launch instead of a bare goroutine, ensuring
AutoProfiler.Done waits for the worker and any active trace to finish; leave the
shutdown await in cmd/ledger unchanged.
---
Outside diff comments:
In `@cmd/ledger/admin.go`:
- Around line 129-162: Restrict the get-config and set-config branches in the
admin command handler, including the set-config flow around
h.configManager.GetField, to loopback callers or authenticated admin requests
before reading or mutating configuration. Reject unauthorized remote requests
with the existing HTTP error mechanism, while preserving access for trusted
local or authenticated admin clients.
---
Nitpick comments:
In `@cmd/ledger/admin.go`:
- Around line 40-57: Add table-driven regression tests covering requireLoopback
rejection of non-loopback and malformed RemoteAddr values, acceptance of
loopback requests, and the get-config/set-config handlers’ success,
validation-error, and internal-error responses. Exercise the existing handler
and configuration symbols directly, asserting HTTP status codes and relevant
response bodies so the new admin boundary and config paths are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f66dda7-7c15-4d4f-af5c-255e7785afe5
📒 Files selected for processing (4)
admin/README.mdcmd/ledger/README.mdcmd/ledger/admin.gocmd/ledger/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- admin/README.md
| logger.Info().Msg("shutting down auto-profiler...") | ||
| <-autoProfiler.Done() | ||
| logger.Info().Msg("auto-profiler stopped") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'main.go|component.go|profiler.go|profiler' . | sed 's#^\./##' | head -80
echo "== cmd/ledger/main.go outline around shutdown =="
if [ -f cmd/ledger/main.go ]; then
wc -l cmd/ledger/main.go
sed -n '340,410p' cmd/ledger/main.go | nl -ba -v340
fi
echo "== locate AutoProfiler and profiler implementation =="
rg -n "type .*AutoProfiler|func .*AutoProfiler|AutoProfiler\.Done|runForever|runOnce|Launch|unit\.Launch|func .*Done" . -S --glob '*.go' | head -200Repository: onflow/flow-go
Length of output: 1959
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg 'module/component/component\.go|cmd/ledger/main\.go|.*profiler\.go$' | sort
echo "== component interface =="
if [ -f module/component/component.go ]; then
sed -n '1,220p' module/component/component.go | nl -ba
fi
echo "== auto-profile lifecycle in cmd/ledger/main.go =="
python3 - <<'PY'
from pathlib import Path
p=Path('cmd/ledger/main.go')
if p.exists():
lines=p.read_text().splitlines()
targets=['NewAutoProfiler','StartAutoProfiler','AutoProfiler','Done()']
for needle in targets:
for i,l in enumerate(lines,1):
if needle in l:
start=max(1,i-20); end=min(len(lines),i+40)
print(f'\n-- {p}:{i} containing {needle} --')
for n in range(start,end+1):
print(f'{n:4}: {lines[n-1]}')
PY
echo "== profiler source =="
for f in $(git ls-files | rg '(^|/)profiler\.go$|auto.*profiler\.go$' | head -20); do
echo "\n== $f =="
wc -l "$f"
ast-grep outline "$f" --view expanded 2>/dev/null || true
sed -n '1,260p' "$f" | nl -ba
doneRepository: onflow/flow-go
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cmd/ledger/main.go relevant sections =="
awk 'NR>=340 && NR<=410 { printf("%4d: %s\n", NR, $0) }' cmd/ledger/main.go
echo "== module/component/component.go =="
awk '{ printf("%4d: %s\n", NR, $0) }' module/component/component.go
echo "== profiler implementation =="
awk '{ printf("%4d: %s\n", NR, $0) }' module/profiler/profiler.go
echo "== references to NewAutoProfiler, StartAutoProfiler, AutoProfiler.Done =="
rg -n "NewAutoProfiler|StartAutoProfiler|AutoProfiler\.Done|runForever|runOnce|Launch\(.*run|unit\.Launch|Done \(\)" cmd/ledger/main.go module/profiler/profiler.go module/component/component.go -SRepository: onflow/flow-go
Length of output: 27351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== unit implementation =="
git ls-files 'module/unit/*.go' 'module/engine/*.go' | rg 'unit|engine' | sort
for f in $(git ls-files 'module/unit/*.go' 'module/engine/*.go' | rg 'unit|engine' | sort); do
echo "\n== $f =="
ast-grep outline "$f" --view expanded 2>/dev/null || true
sed -n '1,260p' "$f" | awk '{ printf("%4d: %s\n", NR, $0) }'
done
echo "== NewAutoProfiler and AutoProfiler usages =="
rg -n "NewAutoProfiler|AutoProfiler|RunComponent|ComponentManagerBuilder|Component" cmd module engine -S --glob '*.go' | head -250Repository: onflow/flow-go
Length of output: 178
🏁 Script executed:
#!/bin/bash
set EUo pipefail
echo "== files containing unit implementation/imports =="
rg -n "type Unit|func .*Unit|func \\(.*\\) Launch|func NewUnit|Shutdown\\(\\).*Done|Done\\(\\).*Quit|Quit\\(\\)" . -S --glob '*.go' | head -200
echo "== go.mod dependencies for unit definitions (if present) =="
rg -n "module|go " go.mod | head -20
echo "== focused grep for NewUnit definitions =="
rg -n "func NewUnit\\(" . -S --glob '*.go'
rg -n "type Unit struct" . -S --glob '*.go'
rg -n "func \\(.*Unit.*\\) Launch|func \\([^ ]*\\) Launch\\(" . -S --glob '*.go'
echo "== NewAutoProfiler and component usages =="
rg -n "NewAutoProfiler|AutoProfiler|RunComponent|ComponentManagerBuilder|ComponentInterface|Component" cmd module engine -S --glob '*.go' | head -250Repository: onflow/flow-go
Length of output: 27908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== engine/unit.go =="
awk '{ printf("%4d: %s\n", NR, $0) }' engine/unit.go
echo "== AutoProfiler construction context =="
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*.go'):
s=str(p)
if 'go' not in s and not s.endswith('.go'):
continue
txt=p.read_text(errors='ignore')
if 'NewAutoProfiler' in txt or 'AutoProfiler' in txt:
lines=txt.splitlines()
for i,l in enumerate(lines,1):
if 'NewAutoProfiler' in l:
for n in range(max(1,i-35), min(len(lines), i+80)+1):
print(f'{p}:{n}: {lines[n-1]}')
break
PYRepository: onflow/flow-go
Length of output: 5084
Track the profiler worker in the unit lifecycle.
AutoProfiler.Done() only waits on the profile directory creation in New() because p.runForever() is started with a bare go before that WaitGroup action is added. Launch the worker with p.unit.Launch(p.runForever) so await in cmd/ledger/main.go waits for the running worker to stop and any active profile trace to complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/ledger/main.go` around lines 384 - 386, Update the auto-profiler startup
in New to launch runForever through p.unit.Launch instead of a bare goroutine,
ensuring AutoProfiler.Done waits for the worker and any active trace to finish;
leave the shutdown await in cmd/ledger unchanged.
Source: Coding guidelines
3 participants
Add the pprof feature to the ledger service through admin tool, so that we can better analyze the memory usage.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
set-configerror handling to return client errors for validation issues.