Skip to content

Add http.Servers timeouts#1064

Merged
simonlechner merged 3 commits into
mainfrom
simon/metrics-timeout
Jul 24, 2026
Merged

Add http.Servers timeouts#1064
simonlechner merged 3 commits into
mainfrom
simon/metrics-timeout

Conversation

@simonlechner

Copy link
Copy Markdown
Contributor

This PR adds timeouts to the standalone pprof and metrics http.Servers.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the standalone pprof and metrics HTTP servers to use explicit http.Server instances with configured timeouts, and adds tests around the new server constructors and endpoint availability.

Changes:

  • Replace http.ListenAndServe calls with configured http.Server instances for pprof and metrics endpoints.
  • Add newPProfServer / newMetricsServer helpers to centralize server configuration (address + timeouts).
  • Add/extend tests to validate timeouts and to confirm key endpoints are served.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
debug/flags.go Introduces newPProfServer and uses it from StartPProf to apply server timeouts.
debug/flags_test.go Adds tests for newPProfServer and verifies pprof/metrics endpoints are reachable.
cmd/sonicd/metrics/flags.go Introduces newMetricsServer and uses it from setupMetricsServer to apply server timeouts.
cmd/sonicd/metrics/flags_test.go Adds a test ensuring newMetricsServer sets expected defaults (addr/handler/timeouts).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread debug/flags.go
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/sonicd/metrics/flags.go 92.85% 0 Missing and 1 partial ⚠️
debug/flags.go 88.88% 0 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
cmd/sonicd/metrics/flags.go 26.08% <92.85%> (+11.08%) ⬆️
debug/flags.go 56.16% <88.88%> (+14.62%) ⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@simonlechner
simonlechner requested a review from LuisPH3 July 14, 2026 08:53
@simonlechner
simonlechner force-pushed the simon/metrics-timeout branch from d9d372b to 873ebfb Compare July 14, 2026 09:39
@simonlechner
simonlechner requested a review from thaarok July 14, 2026 09:39
thaarok
thaarok previously approved these changes Jul 15, 2026

@thaarok thaarok left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if tests TestNewPProfServer_HasTimeouts and TestNewMetricsServer_SetsDefaultValues are meaningful, since they only tests assigning into a structure, but if it is required by the coverage requirements...

Comment thread cmd/sonicd/metrics/flags.go
simonlechner and others added 3 commits July 24, 2026 09:55
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@simonlechner
simonlechner merged commit 807fa1e into main Jul 24, 2026
3 checks passed
@simonlechner
simonlechner deleted the simon/metrics-timeout branch July 24, 2026 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants