fix: set octet-stream Content-Type on pprof endpoints for scraper compatibility#5071
fix: set octet-stream Content-Type on pprof endpoints for scraper compatibility#5071tcx4c70 wants to merge 1 commit into
Conversation
…patibility The custom pprof server wrote profile payloads without setting a Content-Type, so net/http sniffed the gzipped protobuf and labeled responses application/x-gzip. Pyroscope/Grafana Alloy scrapers require application/octet-stream and rejected every profile with "invalid profile data: unexpected Content-Type application/x-gzip". Explicitly set Content-Type (and X-Content-Type-Options: nosniff plus Content-Disposition) before the first write on the Lookup, CPU profile, and trace handlers, matching the standard net/http/pprof behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughResponse headers are added to the pprof profiling HTTP handlers before payload writes. The index handler sets a text Content-Type and nosniff; the named-profile handler sets Content-Type conditionally on the debug parameter plus a Content-Disposition filename; the CPU profile and trace handlers set octet-stream Content-Type, nosniff, and Content-Disposition filenames before streaming. ChangesProfiling response headers
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
Confidence Score: 5/5The changed flow looks mergeable after a small cleanup to error-response headers.
transports/bifrost-http/profiling/profiling.go Important Files Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
transports/bifrost-http/profiling/profiling.go (1)
88-94: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueOptional: clear
Content-Dispositionon error paths.If
pprof.StartCPUProfileortrace.Startfails,http.ErroroverridesContent-Typetotext/plainbut does not remove theContent-Disposition: attachmentheader set just above. The error response would then be downloaded as a file named "profile"/"trace" instead of displayed inline. This is a debug-only endpoint so impact is negligible, but a one-linerw.Header().Del("Content-Disposition")before thehttp.Errorcall would make error responses cleaner.Optional fix for CPU profile handler
if err := pprof.StartCPUProfile(w); err != nil { + w.Header().Del("Content-Disposition") http.Error(w, err.Error(), http.StatusInternalServerError) return }Also applies to: 119-123
🤖 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 `@transports/bifrost-http/profiling/profiling.go` around lines 88 - 94, The profiling handlers in profiling.go leave Content-Disposition set when StartCPUProfile or trace.Start fails, causing http.Error responses to be downloaded as files instead of shown inline. In the CPU and trace error paths inside the profiling handler functions, remove the attachment header before calling http.Error so error responses are clean; use the existing header setup around Content-Disposition in the profile/trace response flow as the place to adjust.
🤖 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 `@transports/bifrost-http/profiling/profiling.go`:
- Around line 88-94: The profiling handlers in profiling.go leave
Content-Disposition set when StartCPUProfile or trace.Start fails, causing
http.Error responses to be downloaded as files instead of shown inline. In the
CPU and trace error paths inside the profiling handler functions, remove the
attachment header before calling http.Error so error responses are clean; use
the existing header setup around Content-Disposition in the profile/trace
response flow as the place to adjust.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9427f2c-13dc-4acc-bd7b-7e048ccc073b
📒 Files selected for processing (1)
transports/bifrost-http/profiling/profiling.go
1 participant
Summary
The custom pprof server wrote profile payloads without setting a Content-Type, so net/http sniffed the gzipped protobuf and labeled responses application/x-gzip. Pyroscope/Grafana Alloy scrapers require application/octet-stream and rejected every profile with "invalid profile data: unexpected Content-Type application/x-gzip".
Changes
Explicitly set Content-Type (and X-Content-Type-Options: nosniff plus Content-Disposition) before the first write on the Lookup, CPU profile, and trace handlers, matching the standard net/http/pprof behavior.
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines