server: add logging for profiling requests (#69897)#69945
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
|
@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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughProfiling requests through HTTP debug routes and ChangesProfiling request logging
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPRouter
participant withProfilingRequestLog
participant ProfilingHandler
participant ZapLogger
Client->>HTTPRouter: Request profiling endpoint
HTTPRouter->>withProfilingRequestLog: Dispatch request
withProfilingRequestLog->>ZapLogger: Record request fields
withProfilingRequestLog->>ProfilingHandler: Invoke handler
ProfilingHandler-->>Client: Return profiling response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 3
🤖 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 @.agents/skills/tidb-test-guidelines/references/server-case-map.md:
- Line 7: Update the server case map overview to include a Testdata section
documenting the ownership of the `testdata/optimizer_suite` data loaded by
`pkg/server/main_test.go` through `LoadTestSuiteData`. Alternatively, remove the
overview’s promise of testdata mappings, but keep the documentation consistent
with the chosen behavior.
In @.agents/skills/tidb-test-guidelines/references/util-case-map.md:
- Line 208: The util-case test-description mappings in the referenced entries
contain mechanically split acronyms; update the descriptions for the symbols
around “is ti flash h t t p resp,” “s e m methods,” “get o s version,” and “get
t z name” to use readable “TiFlash HTTP response,” “SEM methods,” “OS version,”
and “TZ name” wording while preserving the existing mapping structure.
In `@pkg/server/http_status.go`:
- Around line 329-407: Resolve the conflict in the HTTP route registration block
by removing all conflict markers and retaining only branch-compatible changes.
Keep the profiling request logging wrappers and existing intended routes, but
verify that StandbyController, traceeventHandler, and coverage support symbols
exist; otherwise remove or implement those additions consistently. If
/debug/traceevent and /covdata remain, update the HTTP API documentation and
document their StandbyController and TIDB_GOCOVERDIR conditions.
🪄 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: 3cd2e998-6d6d-4027-83e1-d96faf731830
📒 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
| ## pkg/util/engine | ||
|
|
||
| ### Tests | ||
| - `pkg/util/engine/engine_test.go` - util/engine: Tests is ti flash h t t p resp. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix mechanically split acronyms in test descriptions.
Descriptions such as is ti flash h t t p resp, s e m methods, get o s version, and get t z name are not readable. Preserve the intended acronyms and use clear descriptions such as “TiFlash HTTP response,” “SEM methods,” “OS version,” and “TZ name.”
Also applies to: 492-492, 559-559, 601-601
🤖 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 @.agents/skills/tidb-test-guidelines/references/util-case-map.md at line 208,
The util-case test-description mappings in the referenced entries contain
mechanically split acronyms; update the descriptions for the symbols around “is
ti flash h t t p resp,” “s e m methods,” “get o s version,” and “get t z name”
to use readable “TiFlash HTTP response,” “SEM methods,” “OS version,” and “TZ
name” wording while preserving the existing mapping structure.
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #69945 +/- ##
================================================
Coverage ? 55.6684%
================================================
Files ? 1849
Lines ? 666286
Branches ? 0
================================================
Hits ? 370911
Misses ? 267786
Partials ? 27589
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[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 |
|
/retest |
4 participants
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
Tests