[DX-3778] Handle seconds=0 in the admin profile command#22135
Merged
pavel-raykov merged 1 commit intodevelopfrom Apr 23, 2026
Merged
[DX-3778] Handle seconds=0 in the admin profile command#22135pavel-raykov merged 1 commit intodevelopfrom
pavel-raykov merged 1 commit intodevelopfrom
Conversation
Contributor
|
I see you updated files related to
|
Contributor
|
✅ No conflicts with other open PRs targeting |
62fbc37 to
7efecd1
Compare
|
89e016e to
a4863d6
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM
Adjusts the admin profile command so that -seconds 0 is treated as “no seconds query parameter”, enabling static heap profiles (and avoiding invalid seconds=0 behavior in Go’s pprof handlers).
Changes:
- Split vitals into “supports delta” vs “duration-based” groups and only include
profile/tracein the default set whenseconds > 0. - Stop unconditionally appending
?seconds=%dto pprof URIs; only add the query parameter whenseconds > 0. - Update/add testscript coverage for the new vitals list output ordering and the
seconds=0behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
core/cmd/admin_commands.go |
Updates vitals selection and pprof URI construction to omit seconds when it’s 0. |
testdata/scripts/admin/profile/test-seconds.txtar |
Adds a new script to exercise seconds=0 vs seconds>0 behavior. |
testdata/scripts/admin/profile/multi-chain-loopp.txtar |
Updates expected vitals list ordering to match the new logic. |
Areas requiring scrupulous human review:
core/cmd/admin_commands.go: vitals gating/validation aroundseconds > 0and the pprof request URL construction (to ensure intended UX for-seconds 0and correct defaults for other values).
|
jmank88
approved these changes
Apr 23, 2026
george-dorin
approved these changes
Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Before this PR we have always passed seconds as "?seconds=%d" arg to collect the corresponding profile.
After this PR we start to interpret seconds = 0 as "no seconds" argument.
This PR is required to obtain static (non-delta) heap profiles. Before this PR passing "seconds=0" would err in heap profiling - https://go.dev/src/net/http/pprof/pprof.go#L278. After this PR we can now obtain static heap profile. (To make things more complicated passing "seconds=0" to CPU profile would not err and reset profiling duration to 30 seconds.)