Skip to content

refactor(pyroscope): Remove reflection workaround for NewOAuth2RoundTripper - #6099

Merged
korniltsev-grafanista merged 5 commits into
mainfrom
kk/6488-remove-reflection-workaround
Apr 24, 2026
Merged

refactor(pyroscope): Remove reflection workaround for NewOAuth2RoundTripper#6099
korniltsev-grafanista merged 5 commits into
mainfrom
kk/6488-remove-reflection-workaround

Conversation

@korniltsev-grafanista

Copy link
Copy Markdown
Contributor

Summary

  • Remove the reflect+unsafe workaround (reflect_oauth2.go) that was needed to call commonconfig.NewOAuth2RoundTripper with its previously unexported *httpClientOptions parameter
  • Delete the associated layout-verification tests (reflect_oauth2_test.go) that ensured struct compatibility
  • Call commonconfig.NewOAuth2RoundTripper directly using its new variadic ...HTTPClientOption signature
  • Bump prometheus/common to include prometheus/common#898

Details

The upstream NewOAuth2RoundTripper accepted an unexported *httpClientOptions as its fourth parameter, making it uncallable from external packages without reflection. The promhttp2 package maintained a local mirror of the struct and used reflect.NewAt + unsafe.Pointer to pass it through — with extensive tests to assert struct layout, field order, and byte-for-byte default matching against the upstream type.

prometheus/common#898 changed the signature to accept variadic ...HTTPClientOption (an exported interface), matching the pattern already used by NewRoundTripperFromConfigWithContext. This makes the workaround unnecessary.

A new toUpstreamOpts() method on the local httpClientOptions converts fields to upstream option values so that any options passed by callers are correctly forwarded to the OAuth2 round tripper.

Net change: 194 lines deleted, 36 added.

Test plan

  • go test ./internal/component/pyroscope/write/... passes
  • All OAuth2-related tests (TestOAuth2, TestOAuth2WithFile, TestOAuth2WithJWTAuth, etc.) pass

…ripper

prometheus/common#898 changed NewOAuth2RoundTripper to accept variadic
HTTPClientOption instead of the unexported *httpClientOptions, making
the reflection+unsafe workaround unnecessary.

Bump prometheus/common to include the fix and call the upstream function
directly, converting local options via a new toUpstreamOpts helper.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the HTTPClientOption interface with a struct that carries both a
local applicator and the equivalent upstream commonconfig.HTTPClientOption.
This lets newRoundTripperFromConfigWithContext forward options directly to
NewOAuth2RoundTripper instead of reconstructing them from the resolved
httpClientOptions struct via toUpstreamOpts().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Run generate-otel-collector-distro and generate-module-dependencies to
sync collector/ and extension/alloyengine/ go.mod with the updated
prometheus/common dependency.

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

github-actions Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

🔍 Dependency Review

github.com/prometheus/common v0.67.5 -> v0.67.6-0.20260415130754-a4fac5c9b6d9 — ❌ Changes Needed

Breaking/API changes were introduced in the config HTTP client utilities that affect existing call sites.

Key upstream change (in package github.com/prometheus/common/config):

  • NewOAuth2RoundTripper API changed:
    • Before: returned (http.RoundTripper, error) and required an unexported *httpClientOptions.
    • After: returns http.RoundTripper and accepts variadic options ...HTTPClientOption (exported).
  • Upstream now exports HTTPClientOption and WithXxx option builders (e.g., WithDialContextFunc, WithHTTP2Disabled, etc.), removing the need for local reflection hacks.

This PR already adapts local code to that breaking change. If there are any other remaining call sites, they must be updated similarly.

Recommended code updates (shown by this PR and required wherever applicable):

  1. Replace reflection-based wrapper/newOAuth2RoundTripper calls with direct upstream calls and remove error handling:
- rt, err = newOAuth2RoundTripper(oauthCredential, cfg.OAuth2, rt, &opts)
- if err != nil {
-   return nil, fmt.Errorf("unable to create OAuth2RoundTripper: %w", err)
- }
+ rt = commonconfig.NewOAuth2RoundTripper(oauthCredential, cfg.OAuth2, rt, upstreamOpts...)
  1. Update tests to reflect the new signature (no error return):
- rt, err := newOAuth2RoundTripper(secret, &expectedConfig, http.DefaultTransport, &defaultHTTPClientOptions)
- require.NoError(t, err)
+ rt := commonconfig.NewOAuth2RoundTripper(secret, &expectedConfig, http.DefaultTransport)
  1. Replace local “reflection shim” with proper upstream options:
  • Delete the reflection bridge files:
    • internal/component/pyroscope/write/promhttp2/reflect_oauth2.go
    • internal/component/pyroscope/write/promhttp2/reflect_oauth2_test.go
  1. Introduce a local HTTPClientOption that:
  • Applies local behavior (for existing internal option handling).
  • Forwards the equivalent upstream commonconfig.HTTPClientOption to NewOAuth2RoundTripper:
- type HTTPClientOption interface {
-   applyToHTTPClientOptions(options *httpClientOptions)
- }
+ type HTTPClientOption struct {
+   applyLocal func(*httpClientOptions)
+   upstream   commonconfig.HTTPClientOption
+ }
  1. Update local option constructors to carry upstream equivalents:
- func WithHTTP2Disabled() HTTPClientOption {
-   return httpClientOptionFunc(func(opts *httpClientOptions) {
-     opts.http2Enabled = false
-   })
- }
+ func WithHTTP2Disabled() HTTPClientOption {
+   return HTTPClientOption{
+     applyLocal: func(opts *httpClientOptions) { opts.http2Enabled = false },
+     upstream:   commonconfig.WithHTTP2Disabled(),
+   }
+ }
  1. Accumulate and forward upstream options:
- for _, opt := range optFuncs {
-   opt.applyToHTTPClientOptions(&opts)
- }
+ upstreamOpts := make([]commonconfig.HTTPClientOption, 0, len(optFuncs))
+ for _, opt := range optFuncs {
+   opt.applyLocal(&opts)
+   if opt.upstream != nil {
+     upstreamOpts = append(upstreamOpts, opt.upstream)
+   }
+ }

Why this is required

  • Upstream removed the unexported httpClientOptions parameter from NewOAuth2RoundTripper and switched to variadic options with exported HTTPClientOption. Code using the old signature or reflection will fail to compile.
  • Upstream eliminated the error return; callers must remove error handling at call sites.

Evidence (upstream API change)

  • New signature used in this PR:
    • commonconfig.NewOAuth2RoundTripper(secret, cfg, next, upstreamOpts...) http.RoundTripper
  • Old local wrapper (now removed) and tests assumed:
    • (secret, cfg, next, *httpClientOptions) (http.RoundTripper, error)

References

  • Upstream commit introducing exported HTTPClientOption and new NewOAuth2RoundTripper signature (for context): prometheus/common@a4fac5c9b6d9
  • Package: github.com/prometheus/common/config

Notes

  • The upgrade pins a pseudo-version (v0.67.6-0.20260415130754-a4fac5c9b6d9). Consider moving to the next tagged release when available for stability.
  • No other dependency changes were included in this diff.

@marcsanmi marcsanmi 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.

LGTM

@korniltsev-grafanista
korniltsev-grafanista enabled auto-merge (squash) April 24, 2026 10:27
@korniltsev-grafanista
korniltsev-grafanista merged commit 7702609 into main Apr 24, 2026
54 of 55 checks passed
@korniltsev-grafanista
korniltsev-grafanista deleted the kk/6488-remove-reflection-workaround branch April 24, 2026 10:39
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants