server: add logging for profiling requests#69897
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProfiling requests through pprof, debug zip, and TiDB profile tables now emit structured logs. Tests capture and verify these logs, Bazel dependencies are updated, test documentation is refreshed, and bindinfo test sharding increases from 25 to 26. ChangesProfiling request logging
Bindinfo test sharding
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPProfiler
participant ZapLogger
participant ProfileTable
Client->>HTTPProfiler: Request pprof or debug zip
HTTPProfiler->>ZapLogger: Record profiling request metadata
HTTPProfiler-->>Client: Return profiling response
Client->>ProfileTable: Query tidb_profile table
ProfileTable->>ZapLogger: Record table and connection metadata
ProfileTable-->>Client: Return profile rows
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/server/handler/tests/http_handler_serial_test.go (1)
487-488: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce the profile duration to speed up the test.
The test route
/debug/pprof/profile?seconds=5(defined earlier indebugRoutes) makes the test suite wait for 5 seconds. To save test execution time without losing coverage, consider reducing this duration to1second. You will need to update both the route definition and this assertion.⚡ Proposed fixes to reduce test wait time
Update the assertion:
- require.Len(t, pprofLogs.FilterField(zap.String("path", "/debug/pprof/profile")). - FilterField(zap.String("seconds", "5")).All(), 1) + require.Len(t, pprofLogs.FilterField(zap.String("path", "/debug/pprof/profile")). + FilterField(zap.String("seconds", "1")).All(), 1)And update the corresponding route definition earlier in
debugRoutes:- "/debug/pprof/profile?seconds=5", + "/debug/pprof/profile?seconds=1",🤖 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 `@pkg/server/handler/tests/http_handler_serial_test.go` around lines 487 - 488, Reduce the pprof profile duration from 5 seconds to 1 second in the debugRoutes definition, and update the corresponding assertion in the profile test to expect seconds="1"; keep the existing route coverage and log filtering intact.pkg/server/http_status.go (1)
364-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the
/debug/zipendpoint which also collects profiles.Since the
/debug/zipendpoint (defined around line 464) internally triggers CPU, heap, and goroutine profile collections which consume significant resources, consider wrapping it withwithPProfRequestLogas well to ensure these collections are properly logged.🛠️ Proposed fix for the `/debug/zip` handler
Update the
/debug/ziphandler registration:- router.HandleFunc("/debug/zip", func(w http.ResponseWriter, r *http.Request) { + router.HandleFunc("/debug/zip", withPProfRequestLog(func(w http.ResponseWriter, r *http.Request) {(Note: If
/debug/zipis ever uncommented in thedebugRoutesslice withinhttp_handler_serial_test.go, thestrings.HasPrefix(route, "/debug/pprof/")check calculatingexpectedPProfLogswill also need to account for it.)🤖 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 `@pkg/server/http_status.go` around lines 364 - 369, Wrap the /debug/zip handler registration with withPProfRequestLog, matching the existing profiling endpoint registrations, so its profile collection activity is logged. Update the related serial test’s expected PProf log calculation only if /debug/zip is enabled there.
🤖 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.
Nitpick comments:
In `@pkg/server/handler/tests/http_handler_serial_test.go`:
- Around line 487-488: Reduce the pprof profile duration from 5 seconds to 1
second in the debugRoutes definition, and update the corresponding assertion in
the profile test to expect seconds="1"; keep the existing route coverage and log
filtering intact.
In `@pkg/server/http_status.go`:
- Around line 364-369: Wrap the /debug/zip handler registration with
withPProfRequestLog, matching the existing profiling endpoint registrations, so
its profile collection activity is logged. Update the related serial test’s
expected PProf log calculation only if /debug/zip is enabled there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d1a6636-de1a-4ac9-b929-ea26f653452d
📒 Files selected for processing (4)
.agents/skills/tidb-test-guidelines/references/server-case-map.mdpkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/http_handler_serial_test.gopkg/server/http_status.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69897 +/- ##
================================================
- Coverage 76.3206% 75.4948% -0.8259%
================================================
Files 2041 2086 +45
Lines 559973 583840 +23867
================================================
+ Hits 427375 440769 +13394
- Misses 131697 140790 +9093
- Partials 901 2281 +1380
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds structured request logging for /debug/pprof/* endpoints on the status HTTP server, improving observability for when/where profiles are collected (per issue #69896).
Changes:
- Introduce a
withPProfRequestLogwrapper to emit structured fields (method/path/remote-addr + selected query params) for pprof requests. - Wrap all
/debug/pprof/*handlers (explicit routes and thePathPrefixfallback) to ensure consistent logging coverage. - Extend existing debug-route tests to assert the expected pprof log entries are emitted, and update Bazel deps/docs accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/server/http_status.go | Adds the pprof request logging wrapper and applies it to all pprof routes on the status server router. |
| pkg/server/handler/tests/http_handler_serial_test.go | Updates TestDebugRoutes to capture and assert structured pprof request logs via zaptest/observer. |
| pkg/server/handler/tests/BUILD.bazel | Adds the zaptest/observer Bazel dependency for the updated test. |
| .agents/skills/tidb-test-guidelines/references/server-case-map.md | Updates the server test map to reflect debug route logging coverage in http_handler_serial_test.go. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
Signed-off-by: gengliqi <gengliqiii@gmail.com>
gengliqi
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/server/handler/tests/http_handler_test.go (1)
1256-1262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
deferto ensure the response body is closed.If
require.Equalorhttputil.DumpResponsefails and aborts the test,resp.Body.Close()at the end will not be executed, leaking the response body resource. Usingdeferimmediately after ensuring there is no error prevents this.♻️ Proposed refactor
resp, err := ts.FetchStatus("/debug/zip?seconds=1") require.NoError(t, err) + defer resp.Body.Close() require.Equal(t, http.StatusOK, resp.StatusCode) b, err := httputil.DumpResponse(resp, true) require.NoError(t, err) require.Greater(t, len(b), 0) - require.NoError(t, resp.Body.Close())🤖 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 `@pkg/server/handler/tests/http_handler_test.go` around lines 1256 - 1262, Update the response handling in the test after ts.FetchStatus to defer closing resp.Body immediately after confirming the fetch succeeded, before assertions or httputil.DumpResponse; remove the explicit close at the end while preserving the existing status and response-content assertions.
🤖 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.
Nitpick comments:
In `@pkg/server/handler/tests/http_handler_test.go`:
- Around line 1256-1262: Update the response handling in the test after
ts.FetchStatus to defer closing resp.Body immediately after confirming the fetch
succeeded, before assertions or httputil.DumpResponse; remove the explicit close
at the end while preserving the existing status and response-content assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 08fd399f-6734-4bc5-a10c-aa083a3bc8a5
📒 Files selected for processing (4)
.agents/skills/tidb-test-guidelines/references/server-case-map.mdpkg/server/handler/tests/http_handler_serial_test.gopkg/server/handler/tests/http_handler_test.gopkg/server/http_status.go
🚧 Files skipped from review as they are similar to previous changes (3)
- .agents/skills/tidb-test-guidelines/references/server-case-map.md
- pkg/server/http_status.go
- pkg/server/handler/tests/http_handler_serial_test.go
Signed-off-by: gengliqi <gengliqiii@gmail.com>
gengliqi
changed the title
|
/retest |
Signed-off-by: gengliqi <gengliqiii@gmail.com>
ti-chi-bot
Bot
added
size/L
gengliqi
changed the title
Signed-off-by: gengliqi <gengliqiii@gmail.com>
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
Signed-off-by: gengliqi <gengliqiii@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bb7133, time-and-fate, YangKeao The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-8.5 |
|
/cherry-pick release-8.5-20260323-v8.5.5 |
|
@gengliqi: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
@gengliqi: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: close #69896
Problem Summary:
See #69896
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
/debug/pprof/routes, capturing method, path, remote address, and (when present) query parameters likeseconds,debug, andgc./debug/zip.