Add endpointURL for bare IP:port values by bramwelt · Pull Request #12 · linuxfoundation/lfx-v2-access-check

bramwelt · 2026-02-13T01:23:45Z

This pull request improves the OpenTelemetry (OTel) SDK configuration in the pkg/utils/otel.go package to ensure endpoint URLs are always valid, preventing parsing errors when using bare IP:port or hostname values. The changes introduce a helper to normalize endpoints, update all OTel exporter initializations to use the correct URL-based option, and add comprehensive tests to verify this behavior.

Endpoint normalization and SDK configuration:

Added the endpointURL helper function to ensure any OTel endpoint has a proper URL scheme (http:// or https://), fixing the issue where bare IP:port values (like 127.0.0.1:4317) would fail parsing in the OTel SDK. (pkg/utils/otel.go) [1] [2]
Updated all OTel exporter initializations to use WithEndpointURL (instead of WithEndpoint) and ensure the endpoint is normalized before use, covering traces, metrics, and logs for both HTTP and gRPC protocols. (pkg/utils/otel.go) [1] [2] [3] [4] [5] [6]

Testing improvements:

Added unit tests for the new endpointURL helper to verify correct scheme prepending and preservation of existing schemes. (pkg/utils/otel_test.go)
Added an integration-style test to confirm that SetupOTelSDKWithConfig correctly normalizes a bare IP:port endpoint and prevents the OTel SDK parsing error. (pkg/utils/otel_test.go)

Minor cleanup:

Minor formatting/whitespace change in test imports. (pkg/utils/otel_test.go)

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-612 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-612 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Trevor Bramwell <tbramwell@linuxfoundation.org>

coderabbitai · 2026-02-13T01:23:59Z

Walkthrough

OTLP endpoint handling is normalized by introducing a helper function that prepends URL schemes when missing. This helper is applied early during SDK setup, and all exporter constructors are updated to use the normalized endpoint value.

Changes

Cohort / File(s)	Summary
OTLP Endpoint Normalization `pkg/utils/otel.go`	Introduces `endpointURL` helper to add http/https schemes to bare endpoints; normalizes endpoint in `SetupOTelSDKWithConfig` before resource creation; updates HTTP, gRPC, metrics, and logs exporter constructors to use `WithEndpointURL` instead of `WithEndpoint`.
Endpoint Normalization Tests `pkg/utils/otel_test.go`	Adds `TestEndpointURL` to validate scheme normalization across various endpoint formats; adds `TestSetupOTelSDKWithConfig_IPEndpoint` to verify SDK setup correctly handles bare IP:port endpoints via normalization.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: introduction of the endpointURL helper function to handle bare IP:port endpoint values in OpenTelemetry SDK configuration.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, covering the endpoint normalization improvements, exporter updates, and new tests added to the codebase.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into `main`

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch bramwelt/otel-endpoint-fix

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

pkg/utils/otel.go (1)

286-299: Consider IPv6 literal address handling.

The current implementation works well for IPv4 and hostnames. However, IPv6 literal addresses require brackets (e.g., [::1]:4317). An unbracketed IPv6 like ::1:4317 would produce http://::1:4317, which url.Parse may still misinterpret.

If IPv6 endpoints are expected, consider adding a test case and potentially detecting/handling them. If IPv6 support is out of scope, this is fine as-is.
pkg/utils/otel_test.go (1)
293-323: Redundant environment variable in test.

Line 297 sets OTEL_EXPORTER_OTLP_ENDPOINT via t.Setenv, but the test explicitly builds OTelConfig with Endpoint: "127.0.0.1:4317" and passes it directly to SetupOTelSDKWithConfig. The env var is unused in this code path since the config is constructed manually.

Consider removing the t.Setenv call to avoid confusion, or add a comment explaining its purpose if it's intentionally testing SDK fallback behavior.
🔧 Suggested fix
 // TestSetupOTelSDKWithConfig_IPEndpoint verifies that SetupOTelSDKWithConfig
 // normalizes a bare IP:port endpoint to include a scheme, preventing the
 // "first path segment in URL cannot contain colon" error from the SDK.
 func TestSetupOTelSDKWithConfig_IPEndpoint(t *testing.T) {
-	t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "127.0.0.1:4317")
-
 	cfg := OTelConfig{
 		ServiceName:       "test-service",

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

This pull request fixes an issue where bare IP:port endpoint values (e.g., 127.0.0.1:4317) would cause URL parsing errors in the OpenTelemetry SDK. The solution normalizes endpoints by adding proper URL schemes based on the insecure flag before passing them to the SDK.

Changes:

Added endpointURL helper function to prepend http:// or https:// scheme to endpoints that lack one
Updated all OTel exporter initializations (traces, metrics, logs for both HTTP and gRPC) to use WithEndpointURL instead of WithEndpoint
Added comprehensive unit tests for the endpointURL helper and integration test for bare IP:port endpoint handling

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
pkg/utils/otel.go	Added `endpointURL` helper function and endpoint normalization in `SetupOTelSDKWithConfig`; updated all 6 exporter initializations to use `WithEndpointURL`
pkg/utils/otel_test.go	Added unit tests for `endpointURL` helper covering various endpoint formats; added integration test for bare IP:port endpoint; minor import formatting change

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-13T01:27:48Z

pkg/utils/otel_test.go

+func TestEndpointURL(t *testing.T) {
+	tests := []struct {
+		name     string
+		raw      string
+		insecure bool
+		want     string
+	}{
+		{
+			name:     "IP:port insecure",
+			raw:      "127.0.0.1:4317",
+			insecure: true,
+			want:     "http://127.0.0.1:4317",
+		},
+		{
+			name:     "IP:port secure",
+			raw:      "127.0.0.1:4317",
+			insecure: false,
+			want:     "https://127.0.0.1:4317",
+		},
+		{
+			name:     "localhost:port insecure",
+			raw:      "localhost:4317",
+			insecure: true,
+			want:     "http://localhost:4317",
+		},
+		{
+			name:     "hostname without port",
+			raw:      "collector",
+			insecure: true,
+			want:     "http://collector",
+		},
+		{
+			name:     "http URL preserved",
+			raw:      "http://collector.example.com:4318",
+			insecure: false,
+			want:     "http://collector.example.com:4318",
+		},
+		{
+			name:     "https URL preserved",
+			raw:      "https://collector.example.com:4318",
+			insecure: true,
+			want:     "https://collector.example.com:4318",
+		},
+		{
+			name:     "https URL with path preserved",
+			raw:      "https://collector.example.com:4318/v1/traces",
+			insecure: false,
+			want:     "https://collector.example.com:4318/v1/traces",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := endpointURL(tt.raw, tt.insecure)
+			if got != tt.want {
+				t.Errorf("endpointURL(%q, %t) = %q, want %q", tt.raw, tt.insecure, got, tt.want)
+			}
+		})
+	}
+}


Test coverage is missing for IPv6 address formats. IPv6 endpoints use bracket notation like [::1]:4317 or [2001:db8::1]:4317. The endpointURL function should handle these correctly (resulting in http://[::1]:4317), but there are no test cases to verify this behavior. Consider adding test cases for IPv6 addresses to ensure they work correctly.

Copilot · 2026-02-13T01:27:49Z

pkg/utils/otel_test.go

+	t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "127.0.0.1:4317")
+


The t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "127.0.0.1:4317") call appears unnecessary in this test. The test is using SetupOTelSDKWithConfig directly with a config object that already has Endpoint: "127.0.0.1:4317" set. The environment variable would only be used if calling OTelConfigFromEnv(), which this test does not do. This could be confusing for future maintainers. Consider removing this line unless it's testing environment variable interaction specifically.

Suggested change

t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "127.0.0.1:4317")

Copilot · 2026-02-13T01:27:49Z

pkg/utils/otel_test.go

+func TestEndpointURL(t *testing.T) {
+	tests := []struct {
+		name     string
+		raw      string
+		insecure bool
+		want     string
+	}{
+		{
+			name:     "IP:port insecure",
+			raw:      "127.0.0.1:4317",
+			insecure: true,
+			want:     "http://127.0.0.1:4317",
+		},
+		{
+			name:     "IP:port secure",
+			raw:      "127.0.0.1:4317",
+			insecure: false,
+			want:     "https://127.0.0.1:4317",
+		},
+		{
+			name:     "localhost:port insecure",
+			raw:      "localhost:4317",
+			insecure: true,
+			want:     "http://localhost:4317",
+		},
+		{
+			name:     "hostname without port",
+			raw:      "collector",
+			insecure: true,
+			want:     "http://collector",
+		},
+		{
+			name:     "http URL preserved",
+			raw:      "http://collector.example.com:4318",
+			insecure: false,
+			want:     "http://collector.example.com:4318",
+		},
+		{
+			name:     "https URL preserved",
+			raw:      "https://collector.example.com:4318",
+			insecure: true,
+			want:     "https://collector.example.com:4318",
+		},
+		{
+			name:     "https URL with path preserved",
+			raw:      "https://collector.example.com:4318/v1/traces",
+			insecure: false,
+			want:     "https://collector.example.com:4318/v1/traces",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := endpointURL(tt.raw, tt.insecure)
+			if got != tt.want {
+				t.Errorf("endpointURL(%q, %t) = %q, want %q", tt.raw, tt.insecure, got, tt.want)
+			}
+		})
+	}
+}


Test coverage is missing for edge cases including empty string and whitespace-only inputs. While the code at line 194 in otel.go checks for empty endpoint before calling endpointURL, the function itself is not tested with empty or whitespace-only strings. If endpointURL is ever called directly elsewhere, these edge cases could cause issues. Consider adding test cases for:

Empty string: ""

Whitespace-only: " "

Already has scheme but it's unusual: "ftp://example.com:4317"

Copilot · 2026-02-13T01:27:49Z

pkg/utils/otel_test.go

+
 	"testing"
 )



Unnecessary blank line between standard library imports. Both context and testing are standard library packages and should not be separated by a blank line. The Go convention (and goimports tool) groups imports as: standard library imports (no separation), blank line, then third-party imports.

For comparison, see pkg/log/log_test.go lines 6-9 where context, log/slog, and testing are all grouped together without blank lines as they're all from the standard library.

Suggested change

"testing"

)

"testing"

)

bramwelt and others added 2 commits February 12, 2026 14:56

bramwelt requested a review from a team as a code owner February 13, 2026 01:23

Copilot AI review requested due to automatic review settings February 13, 2026 01:23

Copilot started reviewing on behalf of bramwelt February 13, 2026 01:24 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

mauriciozanettisalomao approved these changes Feb 16, 2026

View reviewed changes

bramwelt merged commit 295a503 into main Feb 16, 2026
11 checks passed

bramwelt deleted the bramwelt/otel-endpoint-fix branch February 16, 2026 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Add endpointURL for bare IP:port values#12

Add endpointURL for bare IP:port values#12
bramwelt merged 2 commits intomainfrom
bramwelt/otel-endpoint-fix

bramwelt commented Feb 13, 2026

Uh oh!

coderabbitai bot commented Feb 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

bramwelt commented Feb 13, 2026

Uh oh!

coderabbitai bot commented Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Feb 13, 2026 •

edited

Loading