fix(sdk): endpoint building #23
Conversation
WalkthroughRemoved endpoint normalization and explicit URL-path handling in the OTLP HTTP exporter; switched to using Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant SDK as Traceloop SDK
participant OTLP as OTLP HTTP Exporter
participant Endpoint as Collector/Endpoint
App->>SDK: Initialize tracing (config)
SDK->>SDK: Build headers map (config.Headers + Authorization if APIKey)
SDK->>OTLP: newTraceloopExporter(WithEndpointURL(config.BaseURL), WithHeaders(headers))
OTLP->>Endpoint: Send traces to config.BaseURL (no appended /v1/traces)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 2898828 in 1 minute and 11 seconds. Click for details.
- Reviewed
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. traceloop-sdk/tracing.go:21
- Draft comment:
Removed endpoint sanitization: Ensure config.BaseURL is provided in host:port format since the previous removal of protocol stripping and default port logic might break proper endpoint formatting. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. traceloop-sdk/tracing.go:21
- Draft comment:
Removed the explicit URL path ('/v1/traces'): Verify that the provided BaseURL now includes the correct trace path or that this change is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment violates several rules. It starts with "Verify that..." which is explicitly called out as not useful. It's asking the author to confirm their intention rather than pointing out a clear issue. If the removal of the path was incorrect, it would likely be caught in testing. The author likely knows what they're doing with the API endpoint. Maybe the removal of the path could cause API compatibility issues if the BaseURL doesn't include the correct path. This could be a real concern. While API compatibility is important, asking for verification isn't actionable. If there was a specific known issue, that should be stated directly. The author likely tested this change. Delete this comment as it's merely asking for verification without pointing out a specific issue that needs to be fixed.
Workflow ID: wflow_W2t5jOpxAF3KlqlW
You can customize 
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
traceloop-sdk/tracing.go (2)
55-58: Env var endpoint for HTTP should be a URL, not host:port.Spec and SDK docs treat
OTEL_EXPORTER_OTLP_ENDPOINTas a full base URL;WithEndpointexpects host:port only. UseWithEndpointURL(endpoint)to honor scheme and avoid insecure/HTTPS mismatches. (opentelemetry.io, pkg.go.dev)- otlphttp.WithEndpoint(endpoint), + otlphttp.WithEndpointURL(endpoint),
63-65: gRPC: honor URL-style endpoints from env vars.
otlpgrpc.WithEndpointexpects host:port; env vars typically include scheme (https://collector:4317). Switch toWithEndpointURLto accept URL-form endpoints. (pkg.go.dev)- otlpgrpc.WithEndpoint(endpoint), + otlpgrpc.WithEndpointURL(endpoint),
🧹 Nitpick comments (1)
traceloop-sdk/tracing.go (1)
31-43: Harden header parsing (trim, SplitN, ignore empties).Current parsing breaks on whitespace and values containing '='. Minor refactor improves robustness.
func parseHeaders(headers string) map[string]string { - headersArr := strings.Split(headers, ",") - headersMap := make(map[string]string) - for _, header := range headersArr { - h := strings.Split(header, "=") - - if len(h) == 2 { - headersMap[h[0]] = h[1] - } - } - - return headersMap + if headers == "" { + return map[string]string{} + } + headersMap := make(map[string]string) + for _, header := range strings.Split(headers, ",") { + h := strings.SplitN(header, "=", 2) + if len(h) != 2 { + continue + } + k := strings.TrimSpace(h[0]) + v := strings.TrimSpace(h[1]) + if k == "" { + continue + } + headersMap[k] = v + } + return headersMap }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
traceloop-sdk/tracing.go(1 hunks)
traceloop-sdk/tracing.go
Outdated
| otlphttp.NewClient( | ||
| otlphttp.WithEndpoint(endpoint), | ||
| otlphttp.WithURLPath("/v1/traces"), | ||
| otlphttp.WithEndpoint(config.BaseURL), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use WithEndpointURL for full URLs (fixes https/http handling).
otlphttp.WithEndpoint expects only host:port (no scheme/path). Passing config.BaseURL like "https://..." will misconfigure the exporter. Use WithEndpointURL instead; default path remains /v1/traces unless overridden. (pkg.go.dev)
Apply:
- otlphttp.WithEndpoint(config.BaseURL),
+ otlphttp.WithEndpointURL(config.BaseURL),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| otlphttp.WithEndpoint(config.BaseURL), | |
| otlphttp.WithEndpointURL(config.BaseURL), |
🤖 Prompt for AI Agents
In traceloop-sdk/tracing.go around line 21, otlphttp.WithEndpoint is being
passed config.BaseURL (a full URL like "https://..."), which expects only
host:port; replace this by parsing config.BaseURL into a *url.URL (using
url.Parse and handling the error) and pass that to
otlphttp.WithEndpointURL(parsedURL). Ensure you import net/url, handle parse
errors (return or log) and keep any other options intact so the exporter uses
the correct scheme and path (/v1/traces by default).
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5407bc7 in 2 minutes and 14 seconds. Click for details.
- Reviewed
549lines of code in8files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. go.work:1
- Draft comment:
Upgraded Go version to 1.24.0; verify that all dependencies support the newer version. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify compatibility of dependencies with the new Go version, which falls under the rule of not asking the author to ensure compatibility or verify intentions. It doesn't provide a specific suggestion or point out a specific issue.
2. traceloop-sdk/config.go:12
- Draft comment:
Introduced a 'Headers' field to the Config struct to allow custom HTTP headers. Ensure consumers update their configurations as needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. traceloop-sdk/sdk.go:55
- Draft comment:
Default BaseURL now includes 'https://'; this change ensures that the endpoint is fully qualified for the OTLP exporter. Confirm that this change meets client expectations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is asking the PR author to confirm that the change meets client expectations, which violates the rule against asking for confirmation of intention or ensuring behavior is intended. However, it does provide specific information about a change in the BaseURL, which could be useful if it were rephrased to focus on the technical aspect rather than client expectations.
4. traceloop-sdk/sdk.go:66
- Draft comment:
The error returned by time.ParseDuration is ignored; consider handling it to avoid silent misconfiguration of the polling interval. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. traceloop-sdk/tracing.go:30
- Draft comment:
Using WithEndpointURL requires a fully qualified URL (including the protocol). Ensure that the BaseURL provided in config always includes the scheme. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is informative and suggests ensuring that the BaseURL includes the scheme. However, it doesn't ask the author to confirm their intention or make a specific code suggestion. It seems to be a reminder rather than a direct request for action.
6. traceloop-sdk/version.go:4
- Draft comment:
Version bump to 0.1.2 reflects breaking changes in endpoint handling. Ensure client applications are updated accordingly if they relied on implicit URL normalization. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is discussing a version bump and its implications on client applications. It suggests ensuring that client applications are updated, which falls under asking the PR author to ensure something is tested or verified. This violates the rules.
7. go.work.sum:117
- Draft comment:
Typographical note: The module path 'go.yaml.in/yaml/v3' appears unusual. It seems likely you intended 'gopkg.in/yaml.v3' (consistent with other YAML module paths), so please verify and correct if necessary. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is a dependency-related comment questioning the validity of a module path. According to the rules, we should NOT comment on dependency changes or library versions. The rules explicitly state to ignore anything related to dependencies. Even if the module path looks unusual, we should trust that the author knows what they're doing with dependencies. The module path does look unusual and could be a genuine typo that would cause issues. Maybe this is an important enough issue to break the dependency rule? No - the rules are very clear about not commenting on dependencies. If there's an issue with the dependency, it would be caught by the build system when trying to download/verify modules. Delete this comment as it violates the rule about not commenting on dependencies and dependency changes.
Workflow ID: wflow_U02f7EgUWTdpeUvU
You can customize
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
traceloop-sdk/tracing.go (1)
36-48: Harden OTLP headers env parsing.Current split drops values containing “=” and keeps whitespace. Use SplitN, trim, and percent-decode per Otel spec.
Apply:
func parseHeaders(headers string) map[string]string { - headersArr := strings.Split(headers, ",") + headersArr := strings.Split(headers, ",") headersMap := make(map[string]string) for _, header := range headersArr { - h := strings.Split(header, "=") - - if len(h) == 2 { - headersMap[h[0]] = h[1] - } + h := strings.SplitN(strings.TrimSpace(header), "=", 2) + if len(h) != 2 || h[0] == "" { + continue + } + k := textproto.CanonicalMIMEHeaderKey(strings.TrimSpace(h[0])) + v, err := url.QueryUnescape(strings.TrimSpace(h[1])) + if err != nil { + v = strings.TrimSpace(h[1]) + } + headersMap[k] = v } return headersMap }Also add:
import ( "net/textproto" "net/url" )outside the selected range.
🧹 Nitpick comments (3)
traceloop-sdk/config.go (1)
12-12: Clarify header semantics (case, precedence, reserved names).Please document that:
- Header keys are case-insensitive and will be canonicalized.
- If both
Headers["Authorization"]andAPIKeyare set, which one wins (today APIKey overrides in tracing.go).- Disallow/ignore unsafe or managed headers like
Host,Content-Length, andContent-Type.This avoids surprises for SDK users.
traceloop-sdk/tracing.go (2)
60-62: Support URL-form endpoints from env.
OTEL_EXPORTER_OTLP_ENDPOINTis commonly a full URL for HTTP. PreferWithEndpointURLwhen a scheme is present; fall back toWithEndpointotherwise.Apply:
- otlphttp.WithEndpoint(endpoint), + func() otlphttp.Option { + if strings.Contains(endpoint, "://") { + return otlphttp.WithEndpointURL(endpoint) + } + return otlphttp.WithEndpoint(endpoint) + }(),
68-70: gRPC endpoint: handle scheme/insecure toggles from env.If users set
OTEL_EXPORTER_OTLP_ENDPOINTlikehttp://host:4317,WithEndpointwill reject the scheme. Consider stripping scheme and honoringOTEL_EXPORTER_OTLP_INSECURE=trueto toggle TLS.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
go.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtraceloop-sdk/go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
traceloop-sdk/config.go(1 hunks)traceloop-sdk/go.mod(1 hunks)traceloop-sdk/sdk.go(1 hunks)traceloop-sdk/tracing.go(2 hunks)traceloop-sdk/version.go(1 hunks)
🔇 Additional comments (4)
traceloop-sdk/version.go (1)
4-4: Version bump acknowledged.Ensure release tags and any user-facing docs reflect 0.1.2; consider adding a CHANGELOG entry for the endpoint/headers behavior change.
traceloop-sdk/go.mod (1)
8-14: Otel upgrades: check semconv and exporter API compatibility.You’re on otel 1.37.0 while importing
semconv/v1.17.0in code. That’s fine, but confirm no deprecated symbols are used and thatotlptracehttp.WithEndpointURLis available (it is in these versions).Also applies to: 49-51
traceloop-sdk/sdk.go (1)
55-55: Good: default BaseURL now includes scheme.This aligns with
WithEndpointURLusage and avoids implicit-port confusion.traceloop-sdk/tracing.go (1)
30-32: LGTM: WithEndpointURL + headers.This fixes the scheme/host/path handling for full URLs and centralizes header config.
| go 1.24.0 | ||
|
|
||
| toolchain go1.24.6 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Go 1.24 toolchain: verify CI/build images.
Confirm your CI/docker/tooling uses Go ≥ 1.24.6; otherwise builds will fail.
Run:
🏁 Script executed:
#!/bin/bash
# Verify local and CI images support Go 1.24.6
go versionLength of output: 145
Update Go toolchain to ≥1.24.6 in CI and local environments
go version reports go1.24.1; bump all CI/docker images and any local Go installs to 1.24.6 to match the go.mod toolchain and avoid build failures.
🤖 Prompt for AI Agents
In traceloop-sdk/go.mod around lines 3 to 5, the module specifies toolchain
go1.24.6 but CI and local environments are still using go1.24.1; update all CI
configs, Docker images, and developer setup instructions to use Go 1.24.6 (or
higher) so they match go.mod: change CI pipeline image tags, Dockerfile FROM
lines, any setup scripts or docs that install Go, and verify by running `go
version` in CI to confirm go1.24.6.
| github.com/Masterminds/semver/v3 v3.4.0 // indirect | ||
| github.com/cenkalti/backoff/v4 v4.3.0 // indirect | ||
| github.com/cenkalti/backoff/v5 v5.0.3 // indirect | ||
| github.com/go-logr/logr v1.4.3 // indirect |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Multiple major versions of cenkalti/backoff pulled in.
You have v2 (direct), v4 (indirect), and v5 (indirect). This increases bloat and can confuse readers. If you don’t import github.com/cenkalti/backoff (v2), drop the direct require; otherwise, consider migrating to a single modern major.
Use this to confirm usage and prune:
Also applies to: 37-37
🏁 Script executed:
#!/bin/bash
# Find imports of all backoff module paths
rg -nP -C1 '\bgithub\.com/cenkalti/backoff(/v[45])?\b'
go mod tidyLength of output: 2264
Migrate backoff usage to v5 and drop v2 require
- In traceloop-sdk/utils.go (line 9), change the import from
"github.com/cenkalti/backoff"
to
"github.com/cenkalti/backoff/v5"
and update any usages accordingly. - In traceloop-sdk/go.mod (line 37), remove the direct
github.com/cenkalti/backoff v2.2.1+incompatible
requirement, then rungo mod tidyto prune v2 and v4 entries.
🤖 Prompt for AI Agents
In traceloop-sdk/go.mod (around lines 17-20 and line ~37) and
traceloop-sdk/utils.go (line 9), migrate all backoff imports and uses to v5:
update the import in utils.go from "github.com/cenkalti/backoff" to
"github.com/cenkalti/backoff/v5" and adjust any type/function references to the
v5 package path; then remove the direct require for github.com/cenkalti/backoff
v2.2.1+incompatible from go.mod, ensure only the v5 module is required (remove
v4 indirect if not needed), and run `go mod tidy` to prune old v2/v4 entries and
refresh go.sum.
| headers := make(map[string]string) | ||
| for k, v := range config.Headers { | ||
| headers[k] = v | ||
| } | ||
| if strings.HasPrefix(endpoint, "http://") { | ||
| endpoint = strings.TrimPrefix(endpoint, "http://") | ||
| } | ||
|
|
||
| // Add default HTTPS port if no port specified | ||
| if !strings.Contains(endpoint, ":") { | ||
| endpoint = endpoint + ":443" | ||
|
|
||
| if config.APIKey != "" { | ||
| headers["Authorization"] = fmt.Sprintf("Bearer %s", config.APIKey) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t override user Authorization; canonicalize header keys.
Honor a user-supplied Authorization and canonicalize header names to avoid duplicates due to casing.
Apply:
- headers := make(map[string]string)
- for k, v := range config.Headers {
- headers[k] = v
- }
+ headers := make(map[string]string)
+ for k, v := range config.Headers {
+ ck := textproto.CanonicalMIMEHeaderKey(k)
+ headers[ck] = v
+ }
- if config.APIKey != "" {
- headers["Authorization"] = fmt.Sprintf("Bearer %s", config.APIKey)
- }
+ if config.APIKey != "" {
+ if _, ok := headers["Authorization"]; !ok {
+ headers["Authorization"] = fmt.Sprintf("Bearer %s", config.APIKey)
+ }
+ }And add:
import "net/textproto"outside the selected range.
🤖 Prompt for AI Agents
In traceloop-sdk/tracing.go around lines 18 to 25, the current code copies
headers and unconditionally sets Authorization, which can override a
user-provided Authorization and can create duplicate headers due to casing;
modify the logic to canonicalize header keys using
net/textproto.CanonicalMIMEHeaderKey when copying into the new map, check
whether an Authorization header (case-insensitive) already exists and only set
the Authorization header from config.APIKey if no user-supplied Authorization is
present, and add the import "net/textproto" at the top of the file.
|
|
||
| func (instance *Traceloop) initTracer(ctx context.Context, serviceName string) error { | ||
| exp, err := newOtlpExporter(ctx, instance.config) | ||
| exp, err := newTraceloopExporter(ctx, instance.config) |
There was a problem hiding this comment.
Regression: env-based OTLP overrides no longer honored.
Switching to newTraceloopExporter bypasses newOtlpExporter, so OTEL_EXPORTER_OTLP_* env vars (including protocol=grpc, endpoint, headers) are ignored. This breaks standard Otel overrides.
Apply:
- exp, err := newTraceloopExporter(ctx, instance.config)
+ exp, err := newOtlpExporter(ctx, instance.config)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| exp, err := newTraceloopExporter(ctx, instance.config) | |
| exp, err := newOtlpExporter(ctx, instance.config) |
🤖 Prompt for AI Agents
In traceloop-sdk/tracing.go around line 122, replacing newOtlpExporter with
newTraceloopExporter bypasses standard OTLP env var handling and breaks
OTEL_EXPORTER_OTLP_* overrides; revert to using newOtlpExporter (or have
newTraceloopExporter delegate to it) so
OTEL_EXPORTER_OTLP_PROTOCOL/ENDPOINT/HEADERS are honored: update the code to
call newOtlpExporter(ctx, instance.config) (and then layer any
traceloop-specific options on top) or modify newTraceloopExporter to read and
apply OTEL env-derived options before constructing the exporter.
Important
Refactored endpoint handling in the SDK to remove implicit behaviors, updated configuration and dependencies, and set SDK version to 0.1.2.
newTraceloopExporter()intracing.gonow usesotlphttp.WithEndpointURL()instead ofotlphttp.WithEndpoint().newTraceloopExporter().newTraceloopExporter().Headersfield toConfigstruct inconfig.go.initialize()insdk.gosets defaultBaseURLtohttps://api.traceloop.comif not provided.go.modandgo.work.go.modandgo.sum.0.1.2inversion.go.This description was created by
for 5407bc7. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Refactor
New Features
Chores
User impact