fix(tracing): append /v1/traces to OTLP endpoint per spec#6137
Merged
Conversation
The OTEL spec requires SDKs to append signal-specific paths (/v1/traces for traces) to the base OTEL_EXPORTER_OTLP_ENDPOINT URL. The Go SDK's WithEndpointURL() uses the URL as-is without appending, unlike the JS SDK which auto-appends. This mismatch caused the JS framework to get 404s when /v1/traces was manually added to the shared endpoint secret. Fix resolveEndpoint() to append /v1/traces if not already present, aligning behavior with the JS SDK and OTEL spec. This allows both Go (mcpg) and JS (framework) exporters to share the same base endpoint URL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the tracing OTLP endpoint resolution to align with the OpenTelemetry spec by ensuring the traces signal path (/v1/traces) is appended to the configured OTEL_EXPORTER_OTLP_ENDPOINT base URL, enabling Go and JS exporters to share the same secret without double-appending.
Changes:
- Update
resolveEndpoint()to append/v1/traceswhen not already present. - Add unit tests covering common endpoint shapes (base URL, trailing slash, already-appended, localhost, empty/nil config).
Show a summary per file
| File | Description |
|---|---|
| internal/tracing/provider.go | Modifies endpoint resolution logic to append /v1/traces for OTLP/HTTP exporter configuration. |
| internal/tracing/resolve_endpoint_test.go | Adds table-driven tests for the updated endpoint resolution behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
Comment on lines
+81
to
85
| endpoint := cfg.Endpoint | ||
| // Append /v1/traces if not already present (OTEL spec compliance) | ||
| if !strings.HasSuffix(endpoint, "/v1/traces") { | ||
| endpoint = strings.TrimRight(endpoint, "/") + "/v1/traces" | ||
| } |
Comment on lines
+10
to
+46
| func TestResolveEndpoint_AppendsV1Traces(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| endpoint string | ||
| want string | ||
| }{ | ||
| { | ||
| name: "base URL without path", | ||
| endpoint: "https://o123.ingest.us.sentry.io/api/456/envelope", | ||
| want: "https://o123.ingest.us.sentry.io/api/456/envelope/v1/traces", | ||
| }, | ||
| { | ||
| name: "base URL with trailing slash", | ||
| endpoint: "https://o123.ingest.us.sentry.io/api/456/envelope/", | ||
| want: "https://o123.ingest.us.sentry.io/api/456/envelope/v1/traces", | ||
| }, | ||
| { | ||
| name: "already has /v1/traces", | ||
| endpoint: "https://o123.ingest.us.sentry.io/api/456/envelope/v1/traces", | ||
| want: "https://o123.ingest.us.sentry.io/api/456/envelope/v1/traces", | ||
| }, | ||
| { | ||
| name: "simple localhost endpoint", | ||
| endpoint: "http://localhost:4318", | ||
| want: "http://localhost:4318/v1/traces", | ||
| }, | ||
| { | ||
| name: "localhost with trailing slash", | ||
| endpoint: "http://localhost:4318/", | ||
| want: "http://localhost:4318/v1/traces", | ||
| }, | ||
| { | ||
| name: "empty endpoint", | ||
| endpoint: "", | ||
| want: "", | ||
| }, | ||
| } |
| want string | ||
| }{ | ||
| { | ||
| name: "base URL without path", |
This was referenced May 20, 2026
This was referenced May 20, 2026
lpcox
added a commit
that referenced
this pull request
May 20, 2026
## Context Addresses review feedback from #6137 (merged before review completed). ## Changes 1. **Proper URL parsing** — `resolveEndpoint()` now uses `net/url.Parse` to safely manipulate the path component, preserving query parameters and fragments. Previously used string suffix matching which could break on edge cases like `/v1/traces/` (trailing slash) or URLs with `?query` / `#fragment`. 2. **Additional test coverage** — Added cases for: - `/v1/traces/` (trailing slash normalization) - URLs with query parameters (preserved after path append) - URLs with fragments (preserved after path append) 3. **Renamed misleading test** — Changed `base URL without path` → `URL without /v1/traces` since the endpoint has a non-empty path. ## Behavior - Normalizes trailing slashes on the path before checking for `/v1/traces` - Appends `/v1/traces` to the path only (not after query/fragment) - Falls back to string concatenation if URL parsing fails
This was referenced May 20, 2026
huangyingting
pushed a commit
to repomesh/gh-aw-mcpg
that referenced
this pull request
May 20, 2026
Address review feedback from PR github#6137: 1. Use net/url.Parse instead of string suffix matching to properly handle edge cases (trailing slashes, query params, fragments). The path is normalized before checking/appending /v1/traces, and query/fragment components are preserved. 2. Add test cases for /v1/traces with trailing slash, URLs with query parameters, and URLs with fragments. 3. Rename misleading test case from 'base URL without path' to 'URL without /v1/traces' since the endpoint has a non-empty path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The OTEL spec requires SDKs to append signal-specific paths (
/v1/tracesfor traces) to the baseOTEL_EXPORTER_OTLP_ENDPOINTURL. However, the Go SDK'sWithEndpointURL()uses the URL as-is without appending, unlike the JS SDK which auto-appends.This mismatch meant that when
/v1/traceswas manually added to the shared endpoint secret to make mcpg work, the JS framework got 404s because it auto-appended again →/v1/traces/v1/traces.Fix
resolveEndpoint()now appends/v1/tracesto the configured endpoint if not already present, aligning behavior with the JS SDK and OTEL spec. This allows both Go (mcpg) and JS (framework) exporters to share the same base endpoint URL without path conflicts.Changes
internal/tracing/provider.go—resolveEndpoint()appends/v1/tracesper OTEL specinternal/tracing/resolve_endpoint_test.go— Unit tests covering all casesAfter Merge
Once released, remove
/v1/tracesfrom theGH_AW_OTEL_SENTRY_ENDPOINTsecret so both JS and Go exporters use the same base URL.