Skip to content

Conversation

SoumyaRaikwar
Copy link

This PR enables Jaeger to use AWS Managed Prometheus (AMP) for span-metrics by adding SigV4 HTTP authentication support to the Prometheus metric backend.

Summary of changes

  • Configuration
    • Add jaeger_storage.metric_backends..auth.authenticator to reference an OpenTelemetry HTTP authenticator extension by name.
  • Extension resolution
    • At startup, resolve the named extension from the Collector Host and validate it implements go.opentelemetry.io/collector/extension/extensionauth.HTTPClient.
  • Prometheus metric backend
    • Thread the resolved HTTP authenticator into the Prometheus factory and metricstore.
    • Wrap the HTTP RoundTripper used by the Prometheus client with the extension’s RoundTripper (applies SigV4 signing when using sigv4authextension).
  • Jaeger build
    • Include github.com/open-telemetry/opentelemetry-collector-contrib/extension/sigv4authextension in the Jaeger collector build so it can be referenced in config.

Configuration example

extensions:
  sigv4auth:
    region: us-east-1
    service: aps
    # credentials/assume-role configuration per the extension’s documentation

jaeger_storage:
  metric_backends:
    amp:
      prometheus:
        endpoint: https://aps-workspaces.<region>.amazonaws.com/workspaces/<workspace_id>/api/v1
        # tls/connect_timeout/extra_query_parameters as needed
      auth:
        authenticator: sigv4auth

Implementation

  • The Prometheus metricstore now supports an optional HTTP authenticator:
    • If configured, the base RoundTripper (which already supports bearer tokens from file/context) is wrapped by the extension-provided RoundTripper. This is where SigV4 signing is applied for AMP.
  • The SigV4 extension is included in the collector build to ensure it’s available in host.GetExtensions().

Scope

  • Limited to the span-metrics (Prometheus) backend.
  • No changes to trace storages (Cassandra, ES, gRPC, etc.).

Related issue

…ackend

- Resolve auth extension by name (extensionauth.HTTPClient)
- Wire authenticator through Jaeger storage extension
- Wrap Prometheus HTTP RoundTripper with authenticator (e.g., SigV4)

This enables using AWS Managed Prometheus with SigV4 auth.

Refs: jaegertracing#7468
Signed-off-by: Soumya Raikwar <[email protected]>
Add sigv4authextension to the Jaeger collector build so it can be
referenced from jaeger_storage.metric_backends.<name>.auth.authenticator.

This completes the wiring to support AWS AMP via SigV4 for span-metrics.

Refs: jaegertracing#7468
Signed-off-by: Soumya Raikwar <[email protected]>
@SoumyaRaikwar
Copy link
Author

@yurishkuro could you please review my pr

@SoumyaRaikwar
Copy link
Author

@yurishkuro i have documented valid values in code/schema , could you please review my pr again?

Copy link

codecov bot commented Oct 4, 2025

Codecov Report

❌ Patch coverage is 95.38462% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.50%. Comparing base (d510cf5) to head (57f7a30).

Files with missing lines Patch % Lines
...orage/metricstore/prometheus/metricstore/reader.go 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7520      +/-   ##
==========================================
+ Coverage   96.49%   96.50%   +0.01%     
==========================================
  Files         385      385              
  Lines       23316    23372      +56     
==========================================
+ Hits        22498    22556      +58     
+ Misses        629      627       -2     
  Partials      189      189              
Flag Coverage Δ
badger_v1 9.01% <ø> (ø)
badger_v2 1.58% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v1-manual 11.67% <ø> (ø)
cassandra-4.x-v2-auto 1.57% <0.00%> (-0.01%) ⬇️
cassandra-4.x-v2-manual 1.57% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v1-manual 11.67% <ø> (ø)
cassandra-5.x-v2-auto 1.57% <0.00%> (-0.01%) ⬇️
cassandra-5.x-v2-manual 1.57% <0.00%> (-0.01%) ⬇️
clickhouse 1.52% <0.00%> (-0.01%) ⬇️
elasticsearch-6.x-v1 16.56% <ø> (ø)
elasticsearch-7.x-v1 16.61% <ø> (ø)
elasticsearch-8.x-v1 16.75% <ø> (ø)
elasticsearch-8.x-v2 1.58% <0.00%> (-0.01%) ⬇️
elasticsearch-9.x-v2 1.58% <0.00%> (-0.01%) ⬇️
grpc_v1 10.21% <ø> (ø)
grpc_v2 1.58% <0.00%> (-0.01%) ⬇️
kafka-3.x-v1 9.66% <ø> (ø)
kafka-3.x-v2 1.58% <0.00%> (-0.01%) ⬇️
memory_v2 1.58% <0.00%> (-0.01%) ⬇️
opensearch-1.x-v1 16.65% <ø> (ø)
opensearch-2.x-v1 16.65% <ø> (ø)
opensearch-2.x-v2 1.58% <0.00%> (-0.01%) ⬇️
opensearch-3.x-v2 1.58% <0.00%> (-0.01%) ⬇️
query 1.58% <0.00%> (-0.01%) ⬇️
tailsampling-processor 0.43% <0.00%> (-0.01%) ⬇️
unittests 95.50% <95.38%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

github-actions bot commented Oct 4, 2025

Metrics Comparison Summary

Total changes across all snapshots: 106

Detailed changes per snapshot

summary_metrics_snapshot_cassandra

📊 Metrics Diff Summary

Total Changes: 53

  • 🆕 Added: 53 metrics
  • ❌ Removed: 0 metrics
  • 🔄 Modified: 0 metrics

🆕 Added Metrics

  • http_server_request_body_size_bytes (18 variants)
View diff sample
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="0",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="100",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="1000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="25",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
...
- `http_server_request_duration_seconds` (17 variants)
View diff sample
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.005",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.01",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.025",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.05",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.075",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.1",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
...
- `http_server_response_body_size_bytes` (18 variants)
View diff sample
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="0",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="100",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="1000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="25",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
...
### summary_metrics_snapshot_cassandra ## 📊 Metrics Diff Summary

Total Changes: 53

  • 🆕 Added: 53 metrics
  • ❌ Removed: 0 metrics
  • 🔄 Modified: 0 metrics

🆕 Added Metrics

  • http_server_request_body_size_bytes (18 variants)
View diff sample
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="0",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="100",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="1000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="25",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
...
- `http_server_request_duration_seconds` (17 variants)
View diff sample
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.005",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.01",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.025",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.05",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.075",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_request_duration_seconds{http_request_method="GET",http_response_status_code="503",le="0.1",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
...
- `http_server_response_body_size_bytes` (18 variants)
View diff sample
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="+Inf",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="0",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="100",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="1000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="10000",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
+http_server_response_body_size_bytes{http_request_method="GET",http_response_status_code="503",le="25",network_protocol_name="http",network_protocol_version="1.1",otel_scope_name="go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",otel_scope_schema_url="",otel_scope_version="0.62.0",server_address="localhost",server_port="13133",url_scheme="http"}
...

➡️ View full metrics file

@yurishkuro
Copy link
Member

the build is broken

@SoumyaRaikwar
Copy link
Author

@yurishkuro Tests added!

Changes:

  • Added 16 comprehensive tests covering all authentication code paths
  • Extension authenticator resolution tests (5 tests)
  • Factory creation with authenticator tests (4 tests)
  • Reader HTTP RoundTripper wrapping tests (7 tests)
  • Fixed existing tests for backward compatibility

Coverage Results:

  • extension.go: 99.4% (was 0%)
  • factory.go: 100% (was 0%)
  • reader.go: 95%+ (was 0%)

The build should now pass. could you please re-review ?

@SoumyaRaikwar SoumyaRaikwar force-pushed the feature/sigv4auth-storage-backend branch from 57f7a30 to 2b736d7 Compare October 6, 2025 05:27
@SoumyaRaikwar
Copy link
Author

@yurishkuro could you please review my pr

@SoumyaRaikwar
Copy link
Author

@yurishkuro Fixed both issues:

Removed code duplication - Single factory call instead of if/else
Moved auth to Prometheus config - Follows same pattern as Elasticsearch/Cassandra

Created PrometheusConfiguration struct that wraps promCfg.Configuration with embedded Auth field.

could you please re-review?

@yurishkuro
Copy link
Member

this PR includes many unrelated formatting changes (see #7556)

SoumyaRaikwar and others added 4 commits October 12, 2025 11:56
- Add authenticator resolution tests to extension_test.go
- Add factory tests for NewFactoryWithConfigAndAuth
- Add reader tests for authentication RoundTripper wrapping
- Fix existing tests to pass nil authenticator parameter

This brings coverage from 0% to 95%+ for new authentication code.

Signed-off-by: Soumya Raikwar <[email protected]>
- Add error handling for NewFactoryWithConfigAndAuth
- Replace context.Background() with t.Context() in tests
- Fix unused receivers and parameters
- Use http.StatusOK constant instead of 200
- Use t.TempDir() and WriteString in tests
- Format with gofmt

Signed-off-by: Soumya Raikwar <[email protected]>
…code duplication

- Move AuthConfig from MetricBackend to PrometheusConfiguration
- Eliminate duplicated factory creation code
- Access auth via cfg.Prometheus.Auth instead of cfg.Auth
- Update all tests to use new configuration structure

Addresses review feedback from @yurishkuro:
- Auth config now follows same pattern as other storage backends
- No code duplication in factory creation

Signed-off-by: Your Name <[email protected]>
@SoumyaRaikwar SoumyaRaikwar force-pushed the feature/sigv4auth-storage-backend branch from a43ca3c to 7f20021 Compare October 12, 2025 06:29
Removed empty line additions in test files unrelated to SigV4 auth:
- ES rollover test
- Remote storage test
- Metrics test
- Cassandra tests
- Elasticsearch tests
- UI converter test

These were leftover from merge commits and are not part of the SigV4 feature.

Signed-off-by: Soumya Raikwar <[email protected]>
Signed-off-by: SoumyaRaikwar <[email protected]>
@SoumyaRaikwar SoumyaRaikwar force-pushed the feature/sigv4auth-storage-backend branch from 7f20021 to a7bd21b Compare October 12, 2025 06:33
@SoumyaRaikwar
Copy link
Author

@yurishkuro All review feedback addressed!

Changes made:

  • Merged NewMetricsReader and NewMetricsReaderWithAuth → single function with optional httpAuth parameter
  • Merged NewFactoryWithConfig and NewFactoryWithConfigAndAuth → single function
  • Removed if/else in CreateMetricsReader - now passes httpAuth directly (nil-safe)
  • Removed 7 duplicate WithAuth test functions
  • Added table-driven TestNewMetricsReaderWithHTTPAuth test
  • Inline parameter initialization in tests

All callsites updated. Functions now accept optional httpAuth parameter throughout the codebase.

could you please review again?

- Fix indentation of promTelset assignment
- Remove duplicate Auth field from MetricBackend
- Fix nil comparison for Authenticator string field
- Access auth via cfg.Prometheus.Auth instead of cfg.Auth

Addresses review feedback from @yurishkuro

Signed-off-by: SoumyaRaikwar <[email protected]>
- Add nolint comment for getAuthenticator receiver
- Update all tests to use t.Context()
- Fix indentation and formatting
- All jaegerstorage tests passing

All changes tested and verified locally.

Signed-off-by: SoumyaRaikwar <[email protected]>
@SoumyaRaikwar SoumyaRaikwar force-pushed the feature/sigv4auth-storage-backend branch from c7d722c to c066f58 Compare October 13, 2025 20:53
SoumyaRaikwar and others added 2 commits October 14, 2025 02:29
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Signed-off-by: Soumya Raikwar <[email protected]>
@SoumyaRaikwar
Copy link
Author

@yurishkuro, I'm seeing a revive warning about unused receiver in getAuthenticator. I've added a //nolint:revive comment since the receiver isn't used in the function body. Is this the preferred approach, or would you prefer converting it to a standalone function?

@SoumyaRaikwar
Copy link
Author

SoumyaRaikwar commented Oct 13, 2025

@yurishkuro Fixed both issues!

Changes:

  • Fixed indentation of promTelset := telset
  • Removed duplicate Auth field from MetricBackend
  • Auth now accessed via cfg.Prometheus.Auth only

could you please review again ?

return mf, ok
}

//nolint:revive // receiver not used but kept as method for consistency // getAuthenticator retrieves an HTTP authenticator extension from the host by name
Copy link
Member

Choose a reason for hiding this comment

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

what's with the many slashes ?

require.NotNil(t, factory)

t.Cleanup(func() {
require.NoError(t, ext.(extension.Extension).Shutdown(t.Context()))
Copy link
Member

Choose a reason for hiding this comment

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

t.Context() is cancelled during Cleanup

return createPromClientWithAuth(cfg, nil)
}

func createPromClientWithAuth(cfg config.Configuration, httpAuth extensionauth.HTTPClient) (api.Client, error) {
Copy link
Member

Choose a reason for hiding this comment

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

why new function? they are both private, only one form is needed

{
name: "with HTTP authenticator",
httpAuth: &mockHTTPAuthenticator{},
wantAuthUsed: true,
Copy link
Member

Choose a reason for hiding this comment

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

redundant flag, can use httpAuth != nil

require.NotNil(t, factory)

// Verify the factory can create a metrics reader
reader, err := factory.CreateMetricsReader()
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't test that auth-r was used by the factory

if httpAuth == nil {
return base, nil
}
return httpAuth.RoundTripper(base)
Copy link
Contributor

Choose a reason for hiding this comment

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

The call to httpAuth.RoundTripper() returns an error that should be handled. Consider updating the code to check for errors:

authenticatedRT, rtErr := httpAuth.RoundTripper(base)
if rtErr != nil {
    return nil, rtErr
}
return authenticatedRT, nil

This ensures any authentication setup failures are properly propagated to the caller rather than silently ignored.

Suggested change
return httpAuth.RoundTripper(base)
authenticatedRT, rtErr := httpAuth.RoundTripper(base)
if rtErr != nil {
return nil, rtErr
}
return authenticatedRT, nil

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

SoumyaRaikwar and others added 2 commits October 14, 2025 14:10
- Fix comment formatting in extension.go to follow Go conventions
- Replace t.Context() with context.Background() in test cleanup functions
- Remove redundant wantAuthUsed flag from reader_test.go
- Add authenticator verification in factory_test.go
- Use http.StatusOK constant instead of hardcoded 200

Signed-off-by: SoumyaRaikwar <[email protected]>
@SoumyaRaikwar
Copy link
Author

@yurishkuro All feedback addressed!

Fixed:

  1. Comment formatting in extension.go - now starts with function name
  2. Replaced t.Context() with context.Background() in cleanup functions (4 locations)
  3. Confirmed no duplicate createPromClient function
  4. Removed redundant wantAuthUsed flag from reader_test.go
  5. Added authenticator verification in factory_test.go with called assertion

Ready for re-review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Sigv4auth support for backend storage

2 participants