Conversation
Prepend a URL scheme to bare IP:port endpoints so the OTel SDK can parse them. Use WithEndpointURL consistently for all exporters and update the env var so the SDK reads a valid URL internally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-609 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
WithEndpointURL receives the normalized endpoint explicitly, making the env var mutation unnecessary. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-609 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>
WalkthroughThe changes introduce endpoint URL normalization for OTLP endpoints in the OpenTelemetry SDK setup. A new unexported function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenTelemetry (OTel) SDK setup helpers to accept bare host:port endpoints by normalizing them into URL form (adding http:// or https:// as needed), and switches OTLP exporter initialization to URL-based endpoint options. It also adds tests to validate the normalization behavior.
Changes:
- Normalize
cfg.Endpointvia a newendpointURLhelper before initializing exporters. - Update OTLP trace/metric/log exporter setup to use
WithEndpointURLinstead ofWithEndpoint. - Add unit tests for endpoint normalization plus an integration-style test covering a bare
IP:portendpoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/utils/otel.go | Normalizes endpoints to include a scheme and updates OTLP exporter initialization to use URL-based endpoint options. |
| pkg/utils/otel_test.go | Adds tests for endpointURL and a SetupOTelSDKWithConfig test case for bare IP:port endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| import ( | ||
| "context" | ||
|
|
There was a problem hiding this comment.
The extra blank line inside the standard-library import block isn’t gofmt/goimports style and may cause formatting/lint churn. Consider running gofmt (or remove the empty line) so imports remain consistently formatted.
| t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "127.0.0.1:4317") | ||
|
|
||
| cfg := OTelConfig{ | ||
| ServiceName: "test-service", | ||
| ServiceVersion: "1.0.0", | ||
| Protocol: OTelProtocolGRPC, | ||
| Endpoint: "127.0.0.1:4317", | ||
| Insecure: true, |
There was a problem hiding this comment.
This test sets OTEL_EXPORTER_OTLP_ENDPOINT but then constructs cfg.Endpoint directly, so the env var doesn’t influence the behavior under test. Consider either (1) building cfg via OTelConfigFromEnv() to validate the end-to-end env->cfg->normalization flow, or (2) removing the t.Setenv line to avoid implying env var handling is being tested here.
| // Normalize endpoint to include a URL scheme so WithEndpointURL can | ||
| // parse it. Bare IP:port values like "127.0.0.1:4317" cause url.Parse | ||
| // to fail with "first path segment in URL cannot contain colon". | ||
| if cfg.Endpoint != "" { | ||
| cfg.Endpoint = endpointURL(cfg.Endpoint, cfg.Insecure) | ||
| } |
There was a problem hiding this comment.
The PR description says SetupOTelSDKWithConfig sets OTEL_EXPORTER_OTLP_ENDPOINT after normalizing, but this function only rewrites cfg.Endpoint and never updates the environment variable. Either update the PR description (and the nearby comments) or set the env var here if that behavior is required for SDK/env-based configuration.
| // endpointURL ensures the endpoint has a URL scheme. The OTel SDK internally | ||
| // reads OTEL_EXPORTER_OTLP_ENDPOINT and parses it with url.Parse, which fails | ||
| // for bare IP:port values like "127.0.0.1:4317" with "first path segment in | ||
| // URL cannot contain colon". Prepending a scheme based on the insecure flag | ||
| // produces a valid URL the SDK can parse. |
There was a problem hiding this comment.
The endpointURL doc comment claims the SDK reads OTEL_EXPORTER_OTLP_ENDPOINT and parses it, but in this codepath endpointURL is only used to prepare cfg.Endpoint for WithEndpointURL options. Consider rewording the comment to describe the actual call chain (or implement env var normalization if that’s the intent).
| // endpointURL ensures the endpoint has a URL scheme. The OTel SDK internally | |
| // reads OTEL_EXPORTER_OTLP_ENDPOINT and parses it with url.Parse, which fails | |
| // for bare IP:port values like "127.0.0.1:4317" with "first path segment in | |
| // URL cannot contain colon". Prepending a scheme based on the insecure flag | |
| // produces a valid URL the SDK can parse. | |
| // endpointURL ensures the endpoint has a URL scheme suitable for use with | |
| // OTLP exporter WithEndpointURL options. These helpers expect an endpoint | |
| // value that url.Parse can handle, so bare IP:port values like "127.0.0.1:4317" | |
| // would otherwise fail with "first path segment in URL cannot contain colon". | |
| // Prepending "http://" or "https://" based on the insecure flag yields a valid | |
| // URL while preserving the original host and port. |
| func endpointURL(raw string, insecure bool) string { | ||
| if strings.Contains(raw, "://") { | ||
| return raw | ||
| } | ||
| if insecure { | ||
| return "http://" + raw | ||
| } | ||
| return "https://" + raw |
There was a problem hiding this comment.
endpointURL should probably strings.TrimSpace(raw) before checking/adding a scheme; otherwise values like "127.0.0.1:4317\n" or " localhost:4317" will be turned into an invalid URL and WithEndpointURL will fail to parse.
This pull request improves the handling of OpenTelemetry (OTel) endpoint configuration to ensure compatibility with the OTel SDK, particularly when users provide endpoints as bare IP:port values. The changes introduce normalization of the endpoint to include the appropriate URL scheme, update SDK initialization to use new URL-based options, and add comprehensive tests for these enhancements.
Endpoint normalization and SDK integration:
SetupOTelSDKWithConfigto normalize thecfg.Endpointvalue by prepending an appropriate URL scheme (http://orhttps://) if missing, and set theOTEL_EXPORTER_OTLP_ENDPOINTenvironment variable accordingly. This prevents SDK parse errors when using bare IP:port endpoints.endpointURLhelper function to perform the normalization, choosing the scheme based on theInsecureflag.WithEndpointURLoption instead ofWithEndpoint, ensuring the SDK receives a valid URL. This change applies to traces, metrics, and logs for both HTTP and gRPC protocols. [1] [2] [3] [4] [5] [6]Testing improvements:
endpointURLfunction to verify correct scheme prepending and preservation of existing schemes.SetupOTelSDKWithConfigcorrectly normalizes a bare IP:port endpoint and prevents SDK parse errors.Minor cleanup:
otel_test.go.