Skip to content

feat(pyroscope): Update debuginfo client for HTTP/1.1 upload API#6037

Open
korniltsev-grafanista wants to merge 19 commits intomainfrom
kk/debuginfo-over-h1
Open

feat(pyroscope): Update debuginfo client for HTTP/1.1 upload API#6037
korniltsev-grafanista wants to merge 19 commits intomainfrom
kk/debuginfo-over-h1

Conversation

@korniltsev-grafanista
Copy link
Copy Markdown
Contributor

Updates the Alloy debuginfo upload client to match the new Pyroscope server API (grafana/pyroscope#5046) which replaced the single bidirectional streaming RPC with three HTTP/1.1-compatible endpoints.

New upload flow:

  1. ShouldInitiateUpload — connect-go unary RPC
  2. POST /debuginfo.v1alpha1.DebuginfoService/Upload/{build_id} — plain HTTP, raw body
  3. UploadFinished — connect-go unary RPC

This removes the hard dependency on HTTP/2 (h2/h2c) for debuginfo uploads.

Depends on grafana/pyroscope#5046 (pyroscope/api module needs to be tagged before the replace directive can be swapped for a proper version).

🤖 Generated with Claude Code

@korniltsev-grafanista korniltsev-grafanista changed the title feat(pyroscope): update debuginfo client for HTTP/1.1 upload API feat(pyroscope): Update debuginfo client for HTTP/1.1 upload API Apr 14, 2026
@korniltsev-grafanista korniltsev-grafanista marked this pull request as ready for review April 14, 2026 06:50
@korniltsev-grafanista korniltsev-grafanista requested review from a team as code owners April 14, 2026 06:50
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 14, 2026

🔍 Dependency Review

github.com/grafana/pyroscope/api v1.3.2 → v1.3.2-0.20260414051146-5eb4b919ec16 — ❌ Changes Needed

Breaking API changes in the Debuginfo service require code updates. The bi-directional streaming RPC DebuginfoService.Upload has been removed/replaced with:

  • Unary RPCs:
    • ShouldInitiateUpload(ShouldInitiateUploadRequest) → ShouldInitiateUploadResponse
    • UploadFinished(UploadFinishedRequest) → UploadFinishedResponse
  • An HTTP POST endpoint for the actual payload upload:
    • Path: /debuginfo.v1alpha1.DebuginfoService/Upload/{gnu_build_id}
    • Body: raw binary debug-info content
    • Status: 200 on success

This impacts both client-side uploaders and server-side (proxy) implementations.

Evidence (based on API surface used in this PR):

  • Old usage (removed): debuginfov1alpha1connect.DebuginfoServiceClient.Upload(ctx) (bidi stream)
  • New usage (added): DebuginfoServiceClient.ShouldInitiateUpload(...) and DebuginfoServiceClient.UploadFinished(...) plus a plain HTTP POST for bytes

What you need to change

  1. Client-side Debuginfo uploader: replace bidi streaming with 2 unary calls + an HTTP POST
  • Before (streaming init + stream chunks + close + drain):
- stream := client.Upload(ctx)
- // Send ShouldInitiateUploadRequest on stream
- _ = stream.Send(&debuginfov1alpha1.UploadRequest{ Data: &debuginfov1alpha1.UploadRequest_Init{ ... }})
- resp, _ := stream.Receive()
- if !resp.GetInit().ShouldInitiateUpload { return }
- // Stream file chunks
- for { stream.Send(&debuginfov1alpha1.UploadRequest{ Data: &debuginfov1alpha1.UploadRequest_Chunk{ ... }}) }
- stream.CloseRequest()
- // Drain server responses
- for { _, err := stream.Receive(); if err == io.EOF { break } }
  • After (unary ShouldInitiateUpload → HTTP upload → unary UploadFinished):
+ // Step 1: ShouldInitiateUpload (unary)
+ initResp, err := client.ShouldInitiateUpload(ctx, connect.NewRequest(&debuginfov1alpha1.ShouldInitiateUploadRequest{
+   File: &debuginfov1alpha1.FileMetadata{
+     GnuBuildId: buildID,
+     OtelFileId: fileID.StringNoQuotes(),
+     Name:       fileName,
+     Type:       fileType,
+   },
+ }))
+ if err != nil || !initResp.Msg.ShouldInitiateUpload { return err }

+ // Step 2: HTTP POST upload of bytes to:
+ //   {BaseURL}/debuginfo.v1alpha1.DebuginfoService/Upload/{gnu_build_id}
+ // Use the same HTTP client configured for the endpoint (auth, TLS, etc.)
+ if err := httpUpload(ctx, httpClient, baseURL, buildID, reader); err != nil { return err }

+ // Step 3: UploadFinished (unary)
+ _, err = client.UploadFinished(ctx, connect.NewRequest(&debuginfov1alpha1.UploadFinishedRequest{
+   GnuBuildId: buildID,
+ }))
+ return err
  1. Server-side/Proxy implementation: stop implementing the streaming Upload RPC; implement the two unary RPCs and add an HTTP handler for raw bytes
  • Before (single bidi streaming handler fanning out to downstreams):
- func (c *Component) Upload(ctx context.Context, stream *connect.BidiStream[debuginfov1alpha1.UploadRequest, debuginfov1alpha1.UploadResponse]) error {
-   // recv init, forward to downstreams, recv downstream init, forward chunks, drain, etc.
- }
- // Registered as:
- debuginfov1alpha1connect.RegisterDebuginfoServiceHandler(router, c) // contained Upload implementation
  • After (unary RPCs + an extra HTTP route for the bytes):
+ // Unary init. Forward to downstream.
+ func (c *Component) ShouldInitiateUpload(
+   ctx context.Context,
+   req *connect.Request[debuginfov1alpha1.ShouldInitiateUploadRequest],
+ ) (*connect.Response[debuginfov1alpha1.ShouldInitiateUploadResponse], error) {
+   client, err := c.firstClient()
+   if err != nil { return nil, connect.NewError(connect.CodeUnavailable, err) }
+   return client.ShouldInitiateUpload(ctx, connect.NewRequest(req.Msg.CloneVT()))
+ }

+ // Unary finished. Forward to downstream.
+ func (c *Component) UploadFinished(
+   ctx context.Context,
+   req *connect.Request[debuginfov1alpha1.UploadFinishedRequest],
+ ) (*connect.Response[debuginfov1alpha1.UploadFinishedResponse], error) {
+   client, err := c.firstClient()
+   if err != nil { return nil, connect.NewError(connect.CodeUnavailable, err) }
+   return client.UploadFinished(ctx, connect.NewRequest(req.Msg.CloneVT()))
+ }

+ // Plain HTTP handler that posts raw bytes to the downstream Upload endpoint.
+ func (c *Component) UploadHTTPHandler() http.Handler {
+   return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+     gnuBuildID := mux.Vars(r)["gnu_build_id"]
+     client, err := c.firstClient()
+     if err != nil {
+       http.Error(w, "no downstream", http.StatusServiceUnavailable)
+       return
+     }
+     ctx, cancel := context.WithTimeout(r.Context(), c.debugInfoUploadTimeout)
+     defer cancel()
+     if err := client.Upload(ctx, gnuBuildID, r.Body); err != nil {
+       http.Error(w, fmt.Sprintf("downstream upload: %v", err), http.StatusBadGateway)
+       return
+     }
+     w.WriteHeader(http.StatusOK)
+   })
+ }

+ // Register both unary RPCs via Connect and the HTTP POST endpoint:
+ debuginfov1alpha1connect.RegisterDebuginfoServiceHandler(router, c)
+ router.Handle("/debuginfo.v1alpha1.DebuginfoService/Upload/{gnu_build_id}", c.UploadHTTPHandler()).Methods("POST")
  1. Client wiring: provide both a Connect client and the HTTP client/base URL to perform the upload
  • If your code previously passed around only debuginfov1alpha1connect.DebuginfoServiceClient, you now also need an HTTP client and base URL for the POST. One way is to wrap them:
+ type DebugInfoClient struct {
+   DebuginfoServiceClient debuginfov1alpha1connect.DebuginfoServiceClient
+   HTTPClient             *http.Client
+   BaseURL                string
+   UploadTimeout          time.Duration
+ }
+ func (c *DebugInfoClient) Upload(ctx context.Context, buildID string, body io.Reader) error {
+   req, _ := http.NewRequestWithContext(ctx, "POST",
+     strings.TrimRight(c.BaseURL, "/")+"/debuginfo.v1alpha1.DebuginfoService/Upload/"+url.PathEscape(buildID), body)
+   resp, err := c.HTTPClient.Do(req)
+   if err != nil { return err }
+   defer resp.Body.Close()
+   if resp.StatusCode != http.StatusOK {
+     return fmt.Errorf("upload: HTTP %d", resp.StatusCode)
+   }
+   return nil
+ }
  1. Call sites that exposed/depended on the Connect streaming client type need signature changes
  • For example:
- func (a Appender) DebugInfoClients() []debuginfov1alpha1connect.DebuginfoServiceClient
+ func (a Appender) DebugInfoClients() []*debuginfoclient.Client
  1. Tests: replace streaming tests with unary + HTTP cases
  • Instead of sending an init message and streaming chunks over Connect, update tests to:
    • call ShouldInitiateUpload
    • POST the bytes to /debuginfo.v1alpha1.DebuginfoService/Upload/{gnu_build_id}
    • call UploadFinished
    • assert downstream recorded the init and the uploaded bytes

Why this is required

  • The new pyroscope/api version replaces the streaming upload RPC with a mixed flow: two unary RPCs bracketing an HTTP POST for the bytes. Code still compiled against v1.3.2’s streaming method will not compile or will fail at runtime once the server no longer implements the old method.

Reference snippets (from the PR changes for this upgrade)

  • Client uploader upgrade (connect unary + HTTP POST):
- stream := client.Upload(ctx)
- // send init on stream, receive response, stream chunks...
+ resp, err := client.ShouldInitiateUpload(ctx, connect.NewRequest(&debuginfov1alpha1.ShouldInitiateUploadRequest{ File: fileMeta }))
+ if err != nil { return fmt.Errorf("ShouldInitiateUpload: %w", err) }
+ if !resp.Msg.ShouldInitiateUpload { return nil }
+ if err := client.Upload(ctx, buildID, r); err != nil { return fmt.Errorf("upload: %w", err) }
+ if _, err := client.UploadFinished(ctx, connect.NewRequest(&debuginfov1alpha1.UploadFinishedRequest{ GnuBuildId: buildID })); err != nil {
+   return fmt.Errorf("UploadFinished: %w", err)
+ }
  • Proxy registration and HTTP handler:
+ debuginfov1alpha1connect.RegisterDebuginfoServiceHandler(router, c)  // unary only
+ router.Handle("/debuginfo.v1alpha1.DebuginfoService/Upload/{gnu_build_id}", c.UploadHTTPHandler()).Methods("POST")

Notes

  • This review only covers dependencies changed in go.mod. No other modules were modified.
  • The updated version is a pseudo-version on the v1.3.2 line; despite sharing the tag base, it introduces a breaking surface change for Debuginfo uploads as shown above.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 346ed2f910

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +25 to +30
func (c *Component) firstClient() (debuginfo.Client, error) {
clients := c.getDebugInfoClients()
if len(clients) == 0 {
return connect.NewError(connect.CodeUnavailable, fmt.Errorf("no downstream endpoints available"))
return nil, fmt.Errorf("no downstream endpoints available")
}
return clients[0], nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve debuginfo fan-out across all downstream endpoints

Selecting clients[0] here changes debuginfo proxying from fan-out to single-target routing: ShouldInitiateUpload, the raw Upload POST handler, and UploadFinished now only hit the first downstream client. In any pyroscope.receive_http config with multiple forward_to write targets, profiles are still replicated but debug symbols are uploaded to only one backend, so the others can no longer symbolize frames correctly. This is a functional regression from the previous multi-downstream behavior.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@marcsanmi marcsanmi Apr 16, 2026

Choose a reason for hiding this comment

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

I assume this was done since its trickier because of the the POST handler

Comment on lines 105 to 108
func (c *Component) update(args component.Arguments) (bool, error) {
shutdown := false
newArgs := args.(Arguments)
// required for debug info upload over connect over http2 over http server port
if newArgs.Server.HTTP.HTTP2 == nil {
newArgs.Server.HTTP.HTTP2 = &fnet.HTTP2Config{}
}
newArgs.Server.HTTP.HTTP2.Enabled = true

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep HTTP/2 enabled when serving gRPC on receive_http

Removing the HTTP2.Enabled = true override in update makes this component inherit the default server setting where HTTP/2 is disabled, which breaks callers using the gRPC protocol on the HTTP port (for example connect.WithGRPC() clients) unless every deployment explicitly opts in to HTTP/2. This change broadens impact beyond debuginfo upload and introduces a backward-incompatible protocol regression for existing gRPC ingestion clients.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

@marcsanmi marcsanmi left a comment

Choose a reason for hiding this comment

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

LGTM

}

func (c *DebugInfoClient) Upload(ctx context.Context, buildID string, body io.Reader) error {
uploadURL := strings.TrimRight(c.BaseURL, "/") + "/debuginfo.v1alpha1.DebuginfoService/Upload/" + buildID
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.

Nit: using url.PathEscape(buildID) as a safeguard

Comment on lines +25 to +30
func (c *Component) firstClient() (debuginfo.Client, error) {
clients := c.getDebugInfoClients()
if len(clients) == 0 {
return connect.NewError(connect.CodeUnavailable, fmt.Errorf("no downstream endpoints available"))
return nil, fmt.Errorf("no downstream endpoints available")
}
return clients[0], nil
Copy link
Copy Markdown
Contributor

@marcsanmi marcsanmi Apr 16, 2026

Choose a reason for hiding this comment

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

I assume this was done since its trickier because of the the POST handler

korniltsev-grafanista added a commit that referenced this pull request Apr 17, 2026
Addresses review comment on PR #6037: escape the build_id path
segment as a safeguard when constructing the Upload URL.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
korniltsev-grafanista and others added 14 commits April 17, 2026 14:31
The Pyroscope server debuginfo API was rewritten from a single
bidirectional streaming RPC to three HTTP/1.1-compatible endpoints
(grafana/pyroscope#5046). This updates the Alloy client to match.

The upload flow is now:
1. ShouldInitiateUpload (connect-go unary RPC)
2. POST /debuginfo.v1alpha1.DebuginfoService/Upload/{build_id} (plain HTTP)
3. UploadFinished (connect-go unary RPC)

Key changes:
- Define reporter.Endpoint (ConnectClient + HTTPClient + BaseURL)
- Rename DebugInfoClients() to DebugInfoEndpoints() across all interfaces
- Rewrite attemptUpload() from bidi streaming to 3-step HTTP/1.1 flow
- Rewrite receive_http proxy to forward to first downstream (no fan-out)
- Remove h2c client, HTTP/2 forcing, and chunk-streaming logic
- All test servers use plain HTTP/1.1

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…m build

The previous approach used `type Endpoint = reporter.Endpoint` in
cross-platform code, which imported the linux-only reporter package
and broke macOS/Windows/FreeBSD builds.

Replace with a DebugInfoClient interface that embeds the connect
client and adds an Upload method for plain HTTP POST. The concrete
implementation lives in write.go where the HTTP client is constructed.
No Endpoint struct needed, no cross-platform import of reporter.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Export the concrete DebugInfoClient implementation as write.Client so
tests can use the real type instead of duplicating it as a mock.

The reporter test keeps its own local implementation because importing
write from reporter would create an import cycle.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- debuginfo.DebugInfoClient interface → debuginfo.Client
- debuginfo.Client struct → debuginfo.Uploader
- debuginfo.NewClient → debuginfo.NewUploader
- write.Client → write.DebugInfoClient

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The repo has three go.mod files that depend on pyroscope/api. Only the
root was updated, causing build failures in CI where the other modules
still resolved the old version without ShouldInitiateUpload/UploadFinished.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses review comment on PR #6037: escape the build_id path
segment as a safeguard when constructing the Upload URL.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Moves the struct + Upload method out of package write into a new
write/debuginfoclient package, breaking the cycle that prevented
reporter tests from reusing the production type. Reporter tests now
use debuginfoclient.Client instead of a duplicated test stub.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
korniltsev-grafanista and others added 5 commits April 17, 2026 14:41
Both debuginfo.Client and reporter.DebugInfoClient had exactly one
implementation (*debuginfoclient.Client), so the interfaces were
pure indirection. Removes them and updates all DebugInfoClients()
method signatures to return []*debuginfoclient.Client.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o proxy

Adds pyroscope_receive_http_debuginfo_downstream_calls_total counter
with method (ShouldInitiateUpload|UploadFinished|Upload) and
result (success|failure) labels. Each handler increments once per
call via deferred increment, so "no downstream available" errors are
also counted as failures.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…d block

Collapses the success/error log into the same defer that increments
the metric, so the path is uniform and the "no downstream" failure is
now logged with the method-specific context. firstClient no longer
logs its own error since the caller's defer handles it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extracts recordDownstream(logger, method, err) helper so all three
proxy methods share one line of defer plumbing for the metric +
result log.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

2 participants