server: add logging for profiling requests (#69897) | tidb-test=release-8.5-20260323-v8.5.5 tikv=release-8.5-20260325-v8.5.5 pd=release-8.5-20260323-v8.5.5#69946
Open
ti-chi-bot wants to merge 2 commits into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
13 tasks
Member
Author
|
@gengliqi This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. DetailsInstructions 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. |
ti-chi-bot
Bot
added
size/XXL
📝 WalkthroughWalkthroughProfiling requests through ChangesProfiling observability
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant withProfilingRequestLog
participant pprofHandler
participant ZapLogger
Client->>withProfilingRequestLog: Request /debug/pprof or /debug/zip
withProfilingRequestLog->>ZapLogger: Emit profiling request metadata
withProfilingRequestLog->>pprofHandler: Execute profiling handler
pprofHandler-->>Client: Return profiling response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
Actionable comments posted: 2
🤖 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 `@pkg/server/http_status.go`:
- Around line 329-407: Resolve the merge conflict in the router setup by
removing all conflict markers and retaining the intended additions:
StandbyController wiring, withProfilingRequestLog wrapping for pprof handlers,
/debug/traceevent, and /covdata. Ensure the resulting code is valid Go and
preserves the existing pprof fallback route.
- Around line 329-407: Update the debug HTTP API documentation to list the new
/debug/traceevent and /covdata routes introduced near the profiling handlers.
Document that /covdata is available only when the TIDB_GOCOVERDIR environment
variable is set.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a0906581-7839-42c8-93be-03dfec25e74f
📒 Files selected for processing (10)
.agents/skills/tidb-test-guidelines/references/server-case-map.md.agents/skills/tidb-test-guidelines/references/util-case-map.mdpkg/infoschema/perfschema/BUILD.bazelpkg/infoschema/perfschema/tables.gopkg/server/handler/tests/BUILD.bazelpkg/server/handler/tests/http_handler_serial_test.gopkg/server/handler/tests/http_handler_test.gopkg/server/http_status.gopkg/util/profile/BUILD.bazelpkg/util/profile/profile_test.go
Comment on lines
+329
to
+407
| <<<<<<< HEAD | ||
| router.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) | ||
| router.HandleFunc("/debug/pprof/profile", cpuprofile.ProfileHTTPHandler) | ||
| router.HandleFunc("/debug/pprof/symbol", pprof.Symbol) | ||
| router.HandleFunc("/debug/pprof/trace", pprof.Trace) | ||
| // Other /debug/pprof paths not covered above are redirected to pprof.Index. | ||
| router.PathPrefix("/debug/pprof/").HandlerFunc(pprof.Index) | ||
| ======= | ||
| if s.StandbyController != nil { | ||
| path, handler := s.StandbyController.Handler(s) | ||
| router.PathPrefix(path).Handler(handler) | ||
| } | ||
|
|
||
| router.HandleFunc("/debug/pprof/cmdline", withProfilingRequestLog(pprof.Cmdline)) | ||
| router.HandleFunc("/debug/pprof/profile", withProfilingRequestLog(cpuprofile.ProfileHTTPHandler)) | ||
| router.HandleFunc("/debug/pprof/symbol", withProfilingRequestLog(pprof.Symbol)) | ||
| router.HandleFunc("/debug/pprof/trace", withProfilingRequestLog(pprof.Trace)) | ||
| // Other /debug/pprof paths not covered above are redirected to pprof.Index. | ||
| router.PathPrefix("/debug/pprof/").HandlerFunc(withProfilingRequestLog(pprof.Index)) | ||
| router.HandleFunc("/debug/traceevent", traceeventHandler) | ||
|
|
||
| router.HandleFunc("/covdata", func(writer http.ResponseWriter, _ *http.Request) { | ||
| writer.Header().Set("Content-Type", "application/zip") | ||
| writer.Header().Set("Content-Disposition", "attachment; filename=files.zip") | ||
|
|
||
| dir := os.Getenv("TIDB_GOCOVERDIR") | ||
| if dir == "" { | ||
| serveError(writer, http.StatusInternalServerError, "TIDB_GOCOVERDIR is not set") | ||
| return | ||
| } | ||
| err := coverage.WriteMetaDir(dir) | ||
| if err != nil { | ||
| logutil.BgLogger().Warn("write coverage meta failed", zap.Error(err)) | ||
| serveError(writer, http.StatusInternalServerError, "write coverage meta failed") | ||
| return | ||
| } | ||
| err = coverage.WriteCountersDir(dir) | ||
| if err != nil { | ||
| logutil.BgLogger().Warn("write coverage counters failed", zap.Error(err)) | ||
| serveError(writer, http.StatusInternalServerError, "write coverage counters failed") | ||
| return | ||
| } | ||
|
|
||
| zipWriter := zip.NewWriter(writer) | ||
|
|
||
| err = filepath.Walk(dir, func(file string, fi os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if fi.IsDir() { | ||
| return nil | ||
| } | ||
| relPath, err := filepath.Rel(dir, file) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| writer, err := zipWriter.Create(relPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| srcFile, err := os.Open(filepath.Clean(file)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer func() { | ||
| _ = srcFile.Close() | ||
| }() | ||
| _, err = io.Copy(writer, srcFile) | ||
| return err | ||
| }) | ||
| if err != nil { | ||
| logutil.BgLogger().Warn("zip coverage files failed", zap.Error(err)) | ||
| serveError(writer, http.StatusInternalServerError, "zip coverage files failed") | ||
| return | ||
| } | ||
| err = zipWriter.Close() | ||
| terror.Log(err) | ||
| }) | ||
| >>>>>>> 3332762af45 (server: add logging for profiling requests (#69897)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Unresolved Git merge-conflict markers committed to source.
Lines 329, 336, and 407 still contain literal <<<<<<< HEAD, =======, and >>>>>>> 3332762af45 (...) conflict markers. This is invalid Go syntax and will fail to compile; it matches the PR objectives note that the bot flagged this branch as having unresolved conflicts and put it on hold pending resolution + /unhold.
Resolve the conflict by merging the two sides intentionally (keep withProfilingRequestLog wrapping, the StandbyController wiring, /debug/traceevent, and /covdata as needed) and remove all conflict markers before merging.
🛠 Example resolution direction (verify intent with the incoming commit)
-<<<<<<< HEAD
- router.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline)
- router.HandleFunc("/debug/pprof/profile", cpuprofile.ProfileHTTPHandler)
- router.HandleFunc("/debug/pprof/symbol", pprof.Symbol)
- router.HandleFunc("/debug/pprof/trace", pprof.Trace)
- // Other /debug/pprof paths not covered above are redirected to pprof.Index.
- router.PathPrefix("/debug/pprof/").HandlerFunc(pprof.Index)
-=======
if s.StandbyController != nil {
path, handler := s.StandbyController.Handler(s)
router.PathPrefix(path).Handler(handler)
}
router.HandleFunc("/debug/pprof/cmdline", withProfilingRequestLog(pprof.Cmdline))
router.HandleFunc("/debug/pprof/profile", withProfilingRequestLog(cpuprofile.ProfileHTTPHandler))
router.HandleFunc("/debug/pprof/symbol", withProfilingRequestLog(pprof.Symbol))
router.HandleFunc("/debug/pprof/trace", withProfilingRequestLog(pprof.Trace))
// Other /debug/pprof paths not covered above are redirected to pprof.Index.
router.PathPrefix("/debug/pprof/").HandlerFunc(withProfilingRequestLog(pprof.Index))
router.HandleFunc("/debug/traceevent", traceeventHandler)
// ...rest of the incoming block...
- }
-)
->>>>>>> 3332762af45 (server: add logging for profiling requests (`#69897`))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <<<<<<< HEAD | |
| router.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline) | |
| router.HandleFunc("/debug/pprof/profile", cpuprofile.ProfileHTTPHandler) | |
| router.HandleFunc("/debug/pprof/symbol", pprof.Symbol) | |
| router.HandleFunc("/debug/pprof/trace", pprof.Trace) | |
| // Other /debug/pprof paths not covered above are redirected to pprof.Index. | |
| router.PathPrefix("/debug/pprof/").HandlerFunc(pprof.Index) | |
| ======= | |
| if s.StandbyController != nil { | |
| path, handler := s.StandbyController.Handler(s) | |
| router.PathPrefix(path).Handler(handler) | |
| } | |
| router.HandleFunc("/debug/pprof/cmdline", withProfilingRequestLog(pprof.Cmdline)) | |
| router.HandleFunc("/debug/pprof/profile", withProfilingRequestLog(cpuprofile.ProfileHTTPHandler)) | |
| router.HandleFunc("/debug/pprof/symbol", withProfilingRequestLog(pprof.Symbol)) | |
| router.HandleFunc("/debug/pprof/trace", withProfilingRequestLog(pprof.Trace)) | |
| // Other /debug/pprof paths not covered above are redirected to pprof.Index. | |
| router.PathPrefix("/debug/pprof/").HandlerFunc(withProfilingRequestLog(pprof.Index)) | |
| router.HandleFunc("/debug/traceevent", traceeventHandler) | |
| router.HandleFunc("/covdata", func(writer http.ResponseWriter, _ *http.Request) { | |
| writer.Header().Set("Content-Type", "application/zip") | |
| writer.Header().Set("Content-Disposition", "attachment; filename=files.zip") | |
| dir := os.Getenv("TIDB_GOCOVERDIR") | |
| if dir == "" { | |
| serveError(writer, http.StatusInternalServerError, "TIDB_GOCOVERDIR is not set") | |
| return | |
| } | |
| err := coverage.WriteMetaDir(dir) | |
| if err != nil { | |
| logutil.BgLogger().Warn("write coverage meta failed", zap.Error(err)) | |
| serveError(writer, http.StatusInternalServerError, "write coverage meta failed") | |
| return | |
| } | |
| err = coverage.WriteCountersDir(dir) | |
| if err != nil { | |
| logutil.BgLogger().Warn("write coverage counters failed", zap.Error(err)) | |
| serveError(writer, http.StatusInternalServerError, "write coverage counters failed") | |
| return | |
| } | |
| zipWriter := zip.NewWriter(writer) | |
| err = filepath.Walk(dir, func(file string, fi os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if fi.IsDir() { | |
| return nil | |
| } | |
| relPath, err := filepath.Rel(dir, file) | |
| if err != nil { | |
| return err | |
| } | |
| writer, err := zipWriter.Create(relPath) | |
| if err != nil { | |
| return err | |
| } | |
| srcFile, err := os.Open(filepath.Clean(file)) | |
| if err != nil { | |
| return err | |
| } | |
| defer func() { | |
| _ = srcFile.Close() | |
| }() | |
| _, err = io.Copy(writer, srcFile) | |
| return err | |
| }) | |
| if err != nil { | |
| logutil.BgLogger().Warn("zip coverage files failed", zap.Error(err)) | |
| serveError(writer, http.StatusInternalServerError, "zip coverage files failed") | |
| return | |
| } | |
| err = zipWriter.Close() | |
| terror.Log(err) | |
| }) | |
| >>>>>>> 3332762af45 (server: add logging for profiling requests (#69897)) | |
| if s.StandbyController != nil { | |
| path, handler := s.StandbyController.Handler(s) | |
| router.PathPrefix(path).Handler(handler) | |
| } | |
| router.HandleFunc("/debug/pprof/cmdline", withProfilingRequestLog(pprof.Cmdline)) | |
| router.HandleFunc("/debug/pprof/profile", withProfilingRequestLog(cpuprofile.ProfileHTTPHandler)) | |
| router.HandleFunc("/debug/pprof/symbol", withProfilingRequestLog(pprof.Symbol)) | |
| router.HandleFunc("/debug/pprof/trace", withProfilingRequestLog(pprof.Trace)) | |
| // Other /debug/pprof paths not covered above are redirected to pprof.Index. | |
| router.PathPrefix("/debug/pprof/").HandlerFunc(withProfilingRequestLog(pprof.Index)) | |
| router.HandleFunc("/debug/traceevent", traceeventHandler) | |
| router.HandleFunc("/covdata", func(writer http.ResponseWriter, _ *http.Request) { | |
| writer.Header().Set("Content-Type", "application/zip") | |
| writer.Header().Set("Content-Disposition", "attachment; filename=files.zip") | |
| dir := os.Getenv("TIDB_GOCOVERDIR") | |
| if dir == "" { | |
| serveError(writer, http.StatusInternalServerError, "TIDB_GOCOVERDIR is not set") | |
| return | |
| } | |
| err := coverage.WriteMetaDir(dir) | |
| if err != nil { | |
| logutil.BgLogger().Warn("write coverage meta failed", zap.Error(err)) | |
| serveError(writer, http.StatusInternalServerError, "write coverage meta failed") | |
| return | |
| } | |
| err = coverage.WriteCountersDir(dir) | |
| if err != nil { | |
| logutil.BgLogger().Warn("write coverage counters failed", zap.Error(err)) | |
| serveError(writer, http.StatusInternalServerError, "write coverage counters failed") | |
| return | |
| } | |
| zipWriter := zip.NewWriter(writer) | |
| err = filepath.Walk(dir, func(file string, fi os.FileInfo, err error) error { | |
| if err != nil { | |
| return err | |
| } | |
| if fi.IsDir() { | |
| return nil | |
| } | |
| relPath, err := filepath.Rel(dir, file) | |
| if err != nil { | |
| return err | |
| } | |
| writer, err := zipWriter.Create(relPath) | |
| if err != nil { | |
| return err | |
| } | |
| srcFile, err := os.Open(filepath.Clean(file)) | |
| if err != nil { | |
| return err | |
| } | |
| defer func() { | |
| _ = srcFile.Close() | |
| }() | |
| _, err = io.Copy(writer, srcFile) | |
| return err | |
| }) | |
| if err != nil { | |
| logutil.BgLogger().Warn("zip coverage files failed", zap.Error(err)) | |
| serveError(writer, http.StatusInternalServerError, "zip coverage files failed") | |
| return | |
| } | |
| err = zipWriter.Close() | |
| terror.Log(err) | |
| }) |
🤖 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 329 - 407, Resolve the merge conflict
in the router setup by removing all conflict markers and retaining the intended
additions: StandbyController wiring, withProfilingRequestLog wrapping for pprof
handlers, /debug/traceevent, and /covdata. Ensure the resulting code is valid Go
and preserves the existing pprof fallback route.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'debug/traceevent|covdata|debug/pprof' docs/tidb_http_api.mdRepository: pingcap/tidb
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant doc and route registrations.
git ls-files 'docs/tidb_http_api.md' 'pkg/server/http_status.go' | cat
echo '--- docs snippets ---'
rg -n -C 2 'debug/traceevent|covdata|debug/pprof|debug/zip' docs/tidb_http_api.md || true
echo '--- server snippets ---'
rg -n -C 3 'debug/traceevent|covdata|debug/pprof|debug/zip|withProfilingRequestLog' pkg/server/http_status.go || trueRepository: pingcap/tidb
Length of output: 2706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the HTTP API docs structure around existing debug endpoints.
wc -l docs/tidb_http_api.md
sed -n '1,260p' docs/tidb_http_api.mdRepository: pingcap/tidb
Length of output: 6189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section of the server file around the merged routes.
sed -n '300,430p' pkg/server/http_status.goRepository: pingcap/tidb
Length of output: 5122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the docs section around the debug/zip entry and nearby HTTP API items.
sed -n '560,620p' docs/tidb_http_api.mdRepository: pingcap/tidb
Length of output: 1839
Document the new debug routes in docs/tidb_http_api.md
Add /debug/traceevent and /covdata, and note that /covdata only works when TIDB_GOCOVERDIR is set.
🤖 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 329 - 407, Update the debug HTTP API
documentation to list the new /debug/traceevent and /covdata routes introduced
near the profiling handlers. Document that /covdata is available only when the
TIDB_GOCOVERDIR environment variable is set.
Source: Path instructions
13 tasks
Signed-off-by: gengliqi <gengliqiii@gmail.com>
ti-chi-bot
Bot
added
size/L
Member
Author
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
gengliqi
approved these changes
Jul 20, 2026
|
@gengliqi: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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 kubernetes-sigs/prow repository. |
YangKeao
approved these changes
Jul 20, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5-20260323-v8.5.5 #69946 +/- ##
================================================================
Coverage ? 55.3017%
================================================================
Files ? 1817
Lines ? 651555
Branches ? 0
================================================================
Hits ? 360321
Misses ? 264409
Partials ? 26825
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
wjhuang2016
approved these changes
Jul 20, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gengliqi, wjhuang2016, 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 |
ti-chi-bot
Bot
added
approved
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Contributor
|
/retest |
jebter
changed the title
|
/retest |
1 similar comment
Contributor
|
/retest |
|
/test pull-br-integration-test |
1 similar comment
|
/test pull-br-integration-test |
|
@ti-chi-bot: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
jebter
changed the title
|
/test pull-br-integration-test |
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
5 participants
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.
This is an automated cherry-pick of #69897
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
New Features
Bug Fixes
Tests