HYPERFLEET-789 - feat: add OpenTelemetry span exporter to adapter#87
HYPERFLEET-789 - feat: add OpenTelemetry span exporter to adapter#87xueli181114 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughInitTracer's signature was changed to accept a logger parameter. A new createExporter helper reads OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_EXPORTER_OTLP_PROTOCOL to optionally construct an OTLP exporter (HTTP or gRPC); when no endpoint is set it returns nil. InitTracer uses a background context, conditionally includes sdktrace.WithBatcher(exporter) only if an exporter exists, and calls exporter.Shutdown if resource creation fails after exporter instantiation. The adapter call site was updated to pass the logger. go.mod dependency versions were updated and unit tests for sampling, exporter creation, and InitTracer were added. Sequence DiagramsequenceDiagram
participant Adapter as Adapter Main
participant InitTracer as InitTracer
participant Env as Environment
participant CreateExporter as createExporter
participant Exporter as OTLP Exporter
participant Provider as TracerProvider
Adapter->>InitTracer: InitTracer(log, serviceName, version, sampleRatio)
InitTracer->>Env: Read OTEL_EXPORTER_OTLP_ENDPOINT
InitTracer->>CreateExporter: createExporter(ctx, log)
CreateExporter->>Env: Read OTEL_EXPORTER_OTLP_ENDPOINT
alt endpoint set
CreateExporter->>Env: Read OTEL_EXPORTER_OTLP_PROTOCOL
CreateExporter->>Exporter: Create HTTP or gRPC exporter
CreateExporter-->>InitTracer: return exporter
else no endpoint
CreateExporter-->>InitTracer: return nil
end
InitTracer->>Provider: Create Resource and TracerProvider
alt exporter present
InitTracer->>Provider: Attach sdktrace.WithBatcher(exporter)
end
InitTracer-->>Adapter: return TracerProvider
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/otel/tracer_test.go (2)
12-15: Avoid swallowing logger initialization errors in tests.Ignoring the error can hide test setup failures and lead to misleading panics.
Proposed fix
-func testLogger() logger.Logger { - log, _ := logger.NewLogger(logger.Config{Level: "error", Output: "stdout", Format: "json"}) - return log +func testLogger(t *testing.T) logger.Logger { + t.Helper() + log, err := logger.NewLogger(logger.Config{Level: "error", Output: "stdout", Format: "json"}) + require.NoError(t, err) + return log }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/otel/tracer_test.go` around lines 12 - 15, The test helper testLogger currently ignores the error returned by logger.NewLogger; update testLogger to handle that error instead of swallowing it—either change its signature to accept *testing.T and call t.Fatalf when logger.NewLogger returns a non-nil error, or keep the no-arg signature and panic/log.Fatalf with a clear message including the error; ensure the error from logger.NewLogger is checked and surfaced so failures in logger initialization are not hidden.
58-68: Add tests for protocol selection branches increateExporter.Current coverage only validates the no-endpoint path; it misses
http,grpc, and unknown-protocol fallback behavior introduced by this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/otel/tracer_test.go` around lines 58 - 68, The test only covers the "no endpoint" branch of createExporter; add subtests in TestCreateExporter to exercise the protocol-selection branches by setting envOtelExporterOtlpEndpoint and envOtelExporterProtocol via t.Setenv for "http" and "grpc" to assert that createExporter returns a non-nil exporter and no error, and add a subtest for an unknown protocol (e.g., "foo") to verify the fallback/error behavior; reference createExporter, TestCreateExporter, envOtelExporterOtlpEndpoint, and envOtelExporterProtocol when adding these cases and assert expected exporter/non-nil and error outcomes consistent with the implementation.pkg/otel/tracer.go (1)
81-89: Honor signal-specific OTLP trace environment variables for exporter configuration.The exporter creation is gated on
OTEL_EXPORTER_OTLP_ENDPOINT. If onlyOTEL_EXPORTER_OTLP_TRACES_ENDPOINTis configured, the exporter is not created. Per the OpenTelemetry specification, signal-specific variables should take precedence over generic ones. CheckOTEL_EXPORTER_OTLP_TRACES_ENDPOINTandOTEL_EXPORTER_OTLP_TRACES_PROTOCOLbefore falling back to their generic counterparts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/otel/tracer.go` around lines 81 - 89, The code currently gates OTLP exporter creation on envOtelExporterOtlpEndpoint only; update the logic in tracer.go so it first reads and uses the signal-specific variables envOtelExporterOtlpTracesEndpoint and envOtelExporterOtlpTracesProtocol (falling back to envOtelExporterOtlpEndpoint and envOtelExporterOtlpProtocol only if the traces-specific ones are empty), then use those chosen values when deciding whether to create the sdktrace.SpanExporter and when configuring the exporter; adjust the otlpEndpoint and protocol resolution accordingly so exporter creation honors OTEL_EXPORTER_OTLP_TRACES_* per the OpenTelemetry spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/otel/tracer.go`:
- Around line 106-107: The log line emitting the raw OTLP endpoint in tracer.go
(the log.Infof call that prints otlpEndpoint and protocol) may leak credentials;
update the logging to sanitize the endpoint before logging by parsing
otlpEndpoint (e.g., url.Parse) and removing userinfo and query params, then log
a redacted or minimal form such as scheme://host[:port] or a masked endpoint
(e.g., replace userinfo and query with "***") along with protocol; ensure the
change is applied where otlpEndpoint is referenced in the log.Infof call so no
raw endpoint value is printed.
---
Nitpick comments:
In `@pkg/otel/tracer_test.go`:
- Around line 12-15: The test helper testLogger currently ignores the error
returned by logger.NewLogger; update testLogger to handle that error instead of
swallowing it—either change its signature to accept *testing.T and call t.Fatalf
when logger.NewLogger returns a non-nil error, or keep the no-arg signature and
panic/log.Fatalf with a clear message including the error; ensure the error from
logger.NewLogger is checked and surfaced so failures in logger initialization
are not hidden.
- Around line 58-68: The test only covers the "no endpoint" branch of
createExporter; add subtests in TestCreateExporter to exercise the
protocol-selection branches by setting envOtelExporterOtlpEndpoint and
envOtelExporterProtocol via t.Setenv for "http" and "grpc" to assert that
createExporter returns a non-nil exporter and no error, and add a subtest for an
unknown protocol (e.g., "foo") to verify the fallback/error behavior; reference
createExporter, TestCreateExporter, envOtelExporterOtlpEndpoint, and
envOtelExporterProtocol when adding these cases and assert expected
exporter/non-nil and error outcomes consistent with the implementation.
In `@pkg/otel/tracer.go`:
- Around line 81-89: The code currently gates OTLP exporter creation on
envOtelExporterOtlpEndpoint only; update the logic in tracer.go so it first
reads and uses the signal-specific variables envOtelExporterOtlpTracesEndpoint
and envOtelExporterOtlpTracesProtocol (falling back to
envOtelExporterOtlpEndpoint and envOtelExporterOtlpProtocol only if the
traces-specific ones are empty), then use those chosen values when deciding
whether to create the sdktrace.SpanExporter and when configuring the exporter;
adjust the otlpEndpoint and protocol resolution accordingly so exporter creation
honors OTEL_EXPORTER_OTLP_TRACES_* per the OpenTelemetry spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 501324d8-81a3-483a-822c-1f9606fdc0cd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/adapter/main.gogo.modpkg/otel/tracer.gopkg/otel/tracer_test.go
xueli181114
force-pushed
the
HYPERFLEET-789
branch
2 times, most recently
from
addc637 to
34f56bd
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/otel/tracer.go (1)
106-107:⚠️ Potential issue | 🟠 MajorAvoid logging the raw OTLP endpoint.
Line 106 logs the full endpoint value; this can leak credentials/tokens when URLs include auth/query data. Log only redacted/minimal metadata.
Proposed fix
- log.Infof(ctx, "OTLP trace exporter configured: endpoint=%s protocol=%s", - otlpEndpoint, protocol) + protocolForLog := protocol + if protocolForLog == "" { + protocolForLog = "grpc" + } + log.Infof(ctx, "OTLP trace exporter configured: endpoint_configured=true protocol=%s", + protocolForLog)As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/otel/tracer.go` around lines 106 - 107, The current log call log.Infof(ctx, "OTLP trace exporter configured: endpoint=%s protocol=%s", otlpEndpoint, protocol) prints the raw otlpEndpoint and may leak credentials; update this to log a redacted/minimal endpoint instead (e.g., implement and call a helper like redactEndpoint(otlpEndpoint) or parse with url.Parse and remove User and sensitive query values), then log only the sanitized value and protocol. Ensure you reference the existing otlpEndpoint and protocol variables and keep the log message shape but replace the raw endpoint with the redacted result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/otel/tracer.go`:
- Around line 106-107: The current log call log.Infof(ctx, "OTLP trace exporter
configured: endpoint=%s protocol=%s", otlpEndpoint, protocol) prints the raw
otlpEndpoint and may leak credentials; update this to log a redacted/minimal
endpoint instead (e.g., implement and call a helper like
redactEndpoint(otlpEndpoint) or parse with url.Parse and remove User and
sensitive query values), then log only the sanitized value and protocol. Ensure
you reference the existing otlpEndpoint and protocol variables and keep the log
message shape but replace the raw endpoint with the redacted result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c8a5e31-a507-4dfa-bf38-c50396caf671
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/adapter/main.gogo.modpkg/otel/tracer.gopkg/otel/tracer_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/otel/tracer_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/adapter/main.go
- go.mod
xueli181114
force-pushed
the
HYPERFLEET-789
branch
from
34f56bd to
da9e1d1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/otel/tracer.go (1)
77-108: Well-structured exporter creation with proper fallback handling.Minor observation: when
OTEL_EXPORTER_OTLP_PROTOCOLis unset, line 106 logsprotocol=(empty) rather than the effective protocolgrpc. Consider normalizing for clearer debug logs:Optional: Log effective protocol
switch strings.ToLower(protocol) { case "http", "http/protobuf": exporter, err = otlptracehttp.New(ctx) case "grpc", "": + protocol = "grpc" exporter, err = otlptracegrpc.New(ctx) default: log.Warnf(ctx, "Unrecognized %s value %q, using default grpc", envOtelExporterOtlpProtocol, protocol) + protocol = "grpc" exporter, err = otlptracegrpc.New(ctx) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/otel/tracer.go` around lines 77 - 108, The log and error messages in createExporter use the raw env value for OTEL_EXPORTER_OTLP_PROTOCOL which can be empty; normalize the protocol to its effective value (treat empty as "grpc") before using it in logs and error strings so that messages show the actual protocol in use. In createExporter, after reading protocol := os.Getenv(envOtelExporterOtlpProtocol) set an effectiveProtocol variable (e.g., default to "grpc" when protocol == "") and use effectiveProtocol for the log.Infof call and the wrapped error message while still using the original/normalized value for the switch selection (or use strings.ToLower(effectiveProtocol) for selection) to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/otel/tracer.go`:
- Around line 77-108: The log and error messages in createExporter use the raw
env value for OTEL_EXPORTER_OTLP_PROTOCOL which can be empty; normalize the
protocol to its effective value (treat empty as "grpc") before using it in logs
and error strings so that messages show the actual protocol in use. In
createExporter, after reading protocol := os.Getenv(envOtelExporterOtlpProtocol)
set an effectiveProtocol variable (e.g., default to "grpc" when protocol == "")
and use effectiveProtocol for the log.Infof call and the wrapped error message
while still using the original/normalized value for the switch selection (or use
strings.ToLower(effectiveProtocol) for selection) to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe8a14e3-b4fc-4661-a464-94e6fd8771ca
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/adapter/main.gogo.modpkg/otel/tracer.gopkg/otel/tracer_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/otel/tracer_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/adapter/main.go
- go.mod
xueli181114
force-pushed
the
HYPERFLEET-789
branch
2 times, most recently
from
092e702 to
fb9925b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/otel/tracer.go`:
- Around line 84-92: The OTLP endpoint/protocol selection currently only reads
envOtelExporterOtlpEndpoint and envOtelExporterOtlpProtocol; update the logic in
pkg/otel/tracer.go around otlpEndpoint, protocol and exporter creation to prefer
signal-specific env vars by first checking envOtelExporterOtlpTracesEndpoint and
envOtelExporterOtlpTracesProtocol (if set) and only then falling back to the
generic envOtelExporterOtlpEndpoint and envOtelExporterOtlpProtocol; ensure
otlpEndpoint and protocol variables are assigned from the traces-specific vars
when present before proceeding with exporter (sdktrace.SpanExporter) creation so
trace-only configs are honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81075060-6937-4563-86b9-9a1325578b25
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/adapter/main.gogo.modpkg/otel/tracer.gopkg/otel/tracer_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/otel/tracer_test.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/adapter/main.go
xueli181114
force-pushed
the
HYPERFLEET-789
branch
from
1714f60 to
cb46472
Compare
Add OTLP span exporter so traces are actually sent to a backend instead of being discarded. When OTEL_EXPORTER_OTLP_ENDPOINT is set, uses OTLP gRPC (default) or HTTP exporter. When no endpoint is configured, TracerProvider still generates trace IDs for log correlation. Spans are batched via BatchSpanProcessor for efficient export.
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Left a few comments, nothing blocking, mostly just stuff to tighten up. The exporter cleanup on resource.New failure is a nice touch, and WithBatcher over WithSyncer is the right call for production.
xueli181114
force-pushed
the
HYPERFLEET-789
branch
from
cb46472 to
cd399f5
Compare
…exporter tests Prefer OTEL_EXPORTER_OTLP_TRACES_ENDPOINT and OTEL_EXPORTER_OTLP_TRACES_PROTOCOL over generic variants per OTel spec. Add tests for exporter protocol selection, fallback behavior, and negative sample ratio.
xueli181114
force-pushed
the
HYPERFLEET-789
branch
from
cd399f5 to
4991644
Compare
| // defaultOtlpProtocol is the default OTLP protocol when none is specified. | ||
| // Per OTel spec, the default SHOULD be "http/protobuf". | ||
| // See: https://opentelemetry.io/docs/specs/otel/protocol/exporter/ | ||
| defaultOtlpProtocol = "http/protobuf" |
There was a problem hiding this comment.
Category: Standards
The default here is http/protobuf, but the HyperFleet tracing standard documents grpc
as the default and all its examples use gRPC endpoints (port 4317).
This means if someone deploys using the standard's recommended
OTEL_EXPORTER_OTLP_ENDPOINT=otel-collector.observability.svc:4317 without setting a
protocol, the adapter will attempt HTTP/protobuf against a gRPC port — silently failing.
Two options:
- Update the tracing standard to document
http/protobufas the new default (and update the
example endpoints to port4318) - Keep
grpcas the default here to match the current standard
Either way, code and standard should agree. What do you think?
There was a problem hiding this comment.
Confirmed: the OTel spec (since v1.35.0) changed the default from grpc to http/protobuf. So the adapter is correct here — it follows the current spec.
I've updated the ticket acceptance criteria to align with this: change the default protocol to http/protobuf and update example endpoints from port 4317 to 4318.
| require.NoError(t, err) | ||
| require.NotNil(t, tp) | ||
| assert.NoError(t, tp.Shutdown(context.Background())) | ||
| }) |
There was a problem hiding this comment.
Category: Improvement
TestInitTracer only tests the no-exporter path. The main feature of this PR — registering a
BatchSpanProcessor with the TracerProvider — isn't covered here. TestCreateExporter
validates exporter creation, but doesn't test the full InitTracer → WithBatcher →
Shutdown lifecycle.
Adding a subtest like this would cover the new code path:
t.Run("initializes with exporter when endpoint is set", func(t *testing.T) {
clearOtelEnv(t)
t.Setenv(envOtelExporterOtlpEndpoint, "http://localhost:4318")
tp, err := InitTracer(log, "test-service", "0.0.1", 1.0)
require.NoError(t, err)
require.NotNil(t, tp)
assert.NoError(t, tp.Shutdown(context.Background()))
})3 participants
Summary
pkg/otel/tracer.goso traces are sent to a backend whenOTEL_EXPORTER_OTLP_ENDPOINTis configuredOTEL_EXPORTER_OTLP_PROTOCOLBatchSpanProcessorfor efficient exportlogger.LoggerinInitTracerinstead of creating one internallyChanges
pkg/otel/tracer.go— AddedcreateExporter(), updatedInitTracersignature to accept logger, conditionally registerWithBatcher(exporter)pkg/otel/tracer_test.go— Tests for sample ratio parsing, exporter creation, and TracerProvider initializationcmd/adapter/main.go— Pass logger toInitTracergo.mod/go.sum— Addedotlptracegrpc,otlptracehttpdependenciesTest plan
go test ./pkg/otel/... -v)go test ./...excluding integration)quay.io/xueli/hyperfleet-adapter:dev-6baad90)traceparentextensionSummary by CodeRabbit
New Features
Bug Fixes
Tests