otel: honor standard env vars + load /etc/http-proxy/otel.env - #679
Conversation
Systemd unit had no EnvironmentFile= so cloudinit-supplied OTLP config never reached the process. Also: honor OTEL_EXPORTER_OTLP_ENDPOINT (CUSTOM_OTLP_ENDPOINT still works as a fallback), accept URL-form endpoints via WithEndpointURL, and merge resource with WithFromEnv + WithHost so OTEL_RESOURCE_ATTRIBUTES lands (env wins over the hardcoded fallbacks so deployment identity like service.name is set by the launcher).
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughOpenTelemetry endpoint configuration moved from proxy-provided options to standard environment variables. Systemd services optionally load ChangesOpenTelemetry configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves OpenTelemetry configuration for http-proxy deployments by ensuring standard OTEL environment variables and deployment-provided resource attributes reliably reach the running systemd service and are applied by the SDK.
Changes:
- Add an optional systemd
EnvironmentFile=-/etc/http-proxy/otel.envto both unit variants so cloud-init/deploy tooling can inject OTEL env vars. - Prefer the standard
OTEL_EXPORTER_OTLP_ENDPOINTover legacyCUSTOM_OTLP_ENDPOINTwhen selecting the OTLP endpoint. - Enhance OTel setup to (a) accept URL-form endpoints via
WithEndpointURLand (b) merge env/host-derived resource attributes with code-defined attributes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
otel/otel.go |
Adds resource merging with env/host detection and supports URL-form OTLP endpoints for traces/metrics exporters. |
http_proxy.go |
Updates telemetry endpoint precedence to honor OTEL_EXPORTER_OTLP_ENDPOINT first. |
internal/http-proxy.service |
Loads optional /etc/http-proxy/otel.env so deploy-time OTEL env vars reach the service. |
internal/http-proxy@.service |
Loads optional /etc/http-proxy/otel.env for templated instances as well. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@otel/otel.go`:
- Around line 109-116: Replace the substring-based port check in both the trace
and metric exporter branches with exact endpoint port parsing using
net.SplitHostPort or URL parsing, so only port 443 is treated as secure and
values such as 4430 or IPv6 addresses are handled correctly. Update the logic
around the trace client options and metric exporter setup while preserving the
existing insecure fallback for non-443 endpoints.
- Around line 88-91: Update the resource creation error path around resource.New
to preserve and continue using a non-nil partial base resource, including its
valid environment and detected attributes. Fall back to
resource.NewWithAttributes only when resource detection returns no resource,
while retaining the existing error logging.
- Around line 105-108: Validate opts.Endpoint before calling
otlptracehttp.WithEndpointURL: parse the URL, require the scheme to be exactly
http or https and require a non-empty host, and return the validation error from
the surrounding setup flow instead of appending options or allowing exporter
defaults. Keep valid URL-form endpoints using WithEndpointURL unchanged.
- Around line 105-108: Update the URL-form endpoint handling in the trace and
metric exporter setup to append each signal’s required path (/v1/traces or
/v1/metrics) before passing the result to WithEndpointURL. Preserve the
configured base path, avoid applying the same unsuffixed endpoint to both
exporters, and make the corresponding change in both URL-handling blocks.
- Around line 76-87: Update the detector order in resource.New so
resource.WithHost() runs before resource.WithFromEnv(). Preserve the existing
detectors and ensure environment-derived attributes, including
OTEL_RESOURCE_ATTRIBUTES host.name values, are applied last and override host
detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c1b686ac-a45f-4612-a47b-bfb5bae61c4f
📒 Files selected for processing (4)
http_proxy.gointernal/http-proxy.serviceinternal/http-proxy@.serviceotel/otel.go
verificationOn success, we should see all http-proxy-lantern be subsumed by query WITH sub AS (
SELECT
service_name,
COALESCE(NULLIF(host_name, ''), proxy_track) AS host_id,
COUNTIF(via != '') AS via_rows,
MAX(ts) AS last_ts
FROM `lantern-cloud.teleport.base_table`
WHERE ts >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 6 HOUR)
AND name = 'proxy.io'
GROUP BY service_name, host_id
)
SELECT
service_name,
COUNTIF(via_rows > 0) AS converted,
COUNT(*) AS total_seen_6h,
COUNTIF(last_ts >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 30 MINUTE))
AS live_now,
COUNTIF(via_rows > 0 AND last_ts >=
TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 30 MINUTE)) AS converted_live,
ROUND(COUNTIF(via_rows > 0 AND last_ts >=
TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 30 MINUTE)) /
NULLIF(COUNTIF(last_ts >=
TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 30 MINUTE)), 0), 2) AS pct_live
FROM sub
GROUP BY service_name
ORDER BY total_seen_6h DESCresult [{
"service_name": "vps-proxy",
"converted": "895",
"total_seen_6h": "1058",
"live_now": "801",
"converted_live": "800",
"pct_live": "1.0"
}, {
"service_name": "http-proxy-lantern",
"converted": "0",
"total_seen_6h": "48",
"live_now": "45",
"converted_live": "0",
"pct_live": "0.0"
}]Warning
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
otel/otel.go:121
endpointURLalways appends the OTLP signal path to any URL-form endpoint. If the provided endpoint already includes/v1/tracesor/v1/metrics(a plausible config), this will produce a doubled path like/v1/traces/v1/tracesand break exports. Consider makingendpointURLidempotent by detecting an existing signalPath suffix before appending.
// endpointURL parses a URL-form endpoint and appends the OTLP signal path
// (e.g. "v1/traces") since WithEndpointURL uses the URL path as-is. Callers
// can pass a base URL like https://collector/otlp and end up hitting
// https://collector/otlp/v1/traces. On parse failure, returns the endpoint
// unchanged.
func endpointURL(endpoint, signalPath string) string {
u, err := url.Parse(endpoint)
if err != nil {
log.Errorf("failed to parse OTEL endpoint %q: %v", endpoint, err)
return endpoint
}
return u.JoinPath(signalPath).String()
}
otel/otel.go:86
- The merge comment says the runtime attrs "don't overlap with any env-supplied key today", but
attrsincludesservice.name, which can be set viaOTEL_SERVICE_NAME/OTEL_RESOURCE_ATTRIBUTESand will be overridden by the env resource in the merge. Updating the comment avoids misleading future readers about the overlap/precedence.
// over the OS host detector, and the merge below places base after the
// runtime attrs so env identity also wins over the code's built-in
// fallbacks. The runtime attrs above (proxy.protocol, is_pro, track, ...)
// don't overlap with any env-supplied key today.
Crosse
left a comment
Crosse
left a comment
There was a problem hiding this comment.
I'd lean towards making this a little more opinionated than it is now, honestly. If we require that OTEL_EXPORTER_OTLP_ENDPOINT be set in the environment as a full URL, most of this code collapses into just the resource merging bit in buildResource():
- If there's no scheme, it's an error. (No need for
endpointURL()orisSecureHostPort(), and bothInitGlobalMeterProvider()'s andBuildTracerProvider()'s diff become subtractive-only.) - No need to divine security based on hard-coded port. (No need for
isSecureHostPort().) - No more manually constructing metrics/trace/logs endpoints; we let the OTel SDK handle constructing them. If endpoints must be set explicitly, the SDK already has it covered via
OTEL_EXPORTER_OTLP_{TRACES,METRICS,LOGS}_ENDPOINTenv vars. - Get rid of
CUSTOM_OTLP_ENDPOINTentirely. We're the only consumers of this code, so we can eliminate any uses of that env var in the same PR.
But I'm not going to hold the PR hostage by requesting changes. 🙂 If you think the idea has merit, great. If not, you're free to merge.
No honestly this is great. I preserved backwards compatibility only from apprehension and deference. I'd love to strip this down to be normal idiomatic otel setup and I'll do that. |
Deployment sets OTEL_EXPORTER_OTLP_ENDPOINT via /etc/http-proxy/otel.env (loaded by the systemd unit), and the OTel SDK reads that env var natively — deriving scheme, TLS, and per-signal endpoints without our help. So the endpoint parsing, TLS-by-port heuristic, and CUSTOM_OTLP_ENDPOINT fallback all go away. - Drop Opts.Endpoint, endpointURL(), isSecureHostPort() from otel/. - Drop getTelemetryEndpoint(), defaultTeleportHost, CUSTOM_OTLP_ENDPOINT from http_proxy.go. Without the env var the SDK falls back to localhost:4318 and the exporter fails at runtime — the implicit telemetry.iantem.io:443 default is gone.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
otel/otel.go:150
otlpmetrichttp.WithHeaders(opts.Headers)has the same precedence behavior as traces: providing it (even as nil) overridesOTEL_EXPORTER_OTLP_HEADERS/OTEL_EXPORTER_OTLP_METRICS_HEADERS. As-is, env-provided headers from/etc/http-proxy/otel.envwon’t be applied for metrics exports. GateWithHeaderson a non-empty map.
exp, err := otlpmetrichttp.New(context.Background(),
otlpmetrichttp.WithHeaders(opts.Headers),
otlpmetrichttp.WithTemporalitySelector(func(kind sdkmetric.InstrumentKind) metricdata.Temporality {
switch kind {
case
otel/otel.go:110
otlptracehttp.WithHeaders(opts.Headers)overridesOTEL_EXPORTER_OTLP_HEADERS/OTEL_EXPORTER_OTLP_TRACES_HEADERSfrom the environment; sinceopts.Headersis never set (nil), this effectively disables env-provided auth headers loaded from/etc/http-proxy/otel.env. Only applyWithHeaderswhen the map is non-empty so standard env var headers still work.
This issue also appears on line 146 of the same file.
func BuildTracerProvider(opts *Opts) (*sdktrace.TracerProvider, func()) {
client := otlptracehttp.NewClient(otlptracehttp.WithHeaders(opts.Headers))
exporter, err := otlptrace.New(context.Background(), client)
http_proxy.go:679
- The PR description says
CUSTOM_OTLP_ENDPOINTis kept as a fallback for backwards compatibility, but the current changes remove all handling of it (and no longer set/choose any endpoint in code). This means deployments still usingCUSTOM_OTLP_ENDPOINT(or using host:port values) will silently stop exporting. Consider mappingCUSTOM_OTLP_ENDPOINTinto the standardOTEL_EXPORTER_OTLP_ENDPOINTenv var when the standard vars are unset, normalizing host:port into a URL.
func (p *Proxy) buildOTELOpts(includeProxyName bool) *otel.Opts {
proxyName, provider, dc := p.ProxyName, p.Provider, p.DC
if dc == "" {
// This proxy is running on the old infrastructure, parse the name to get the dc
proxyName, dc = proxyNameAndDC(p.ProxyName)
}
opts := &otel.Opts{
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 (1)
http_proxy.go (1)
679-688: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve environment-derived OTLP headers.
buildOTELOptsleavesotel.Opts.Headersnil. Bothotel/otel.gocallers pass this map explicitly viaWithHeaders, so ifOTEL_EXPORTER_OTLP_HEADERSor signal-specific headers supply required metadata, those headers are dropped for trace and metric exports. Only callWithHeaderswhenopts.Headersis non-nil, or merge explicit headers with the environment-derived header map.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@http_proxy.go` around lines 679 - 688, Update buildOTELOpts to preserve environment-derived OTLP headers by populating or merging opts.Headers with the headers used by the otel/otel.go callers; ensure WithHeaders is invoked only when opts.Headers is non-nil so trace and metric exports retain environment and signal-specific metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@http_proxy.go`:
- Line 640: Update the OTEL initialization paths around BuildTracerProvider and
configureOTELMetrics to require OTEL_EXPORTER_OTLP_ENDPOINT before starting
http-proxy. Validate the value from the systemd environment and surface a clear
deployment error when it is absent, ensuring the exporter cannot silently fall
back to https://localhost:4318.
---
Outside diff comments:
In `@http_proxy.go`:
- Around line 679-688: Update buildOTELOpts to preserve environment-derived OTLP
headers by populating or merging opts.Headers with the headers used by the
otel/otel.go callers; ensure WithHeaders is invoked only when opts.Headers is
non-nil so trace and metric exports retain environment and signal-specific
metadata.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e7d1126c-ceb3-47b6-8754-ba293b46f501
📒 Files selected for processing (2)
http_proxy.gootel/otel.go
🚧 Files skipped from review as they are similar to previous changes (1)
- otel/otel.go
Removing the telemetry.iantem.io:443 default without an assertion turned a misconfiguration into a silent telemetry outage — otlptracehttp / otlpmetrichttp default to localhost:4318 and construction never fails. Log the resolved endpoint at startup, and log a loud error if neither the per-signal nor the generic OTEL_EXPORTER_OTLP_ENDPOINT env var is set. Also drop the unused Opts.Headers field. Headers now come from the standard OTEL_EXPORTER_OTLP_HEADERS env var, same as the endpoint.
The prior comment claimed the runtime attrs "don't overlap with any env-supplied key today," but service.name is exactly such an overlap (and env winning is intentional). Reword to be honest about the one overlap and clarify which attrs actually can't collide.
relates to:
problem
http-proxy still ships directly to teleport, not routing through
opslike we want.why
Systemd unit had no
EnvironmentFile=so cloudinit-supplied OTLP env vars never reached the process. Every deployment that thought it was pointing http-proxy-lantern at a local otelcol was in fact getting the hardcodedtelemetry.iantem.io:443fallback with no resource attribution.fixes
internal/http-proxy{,@}.service:EnvironmentFile=-/etc/http-proxy/otel.env(leading-= optional).http_proxy.go:getTelemetryEndpoint: checkOTEL_EXPORTER_OTLP_ENDPOINTfirst;CUSTOM_OTLP_ENDPOINTkept as fallback for backwards compat.otel/otel.go: accept URL-form endpoints viaWithEndpointURL; merge resource withresource.WithFromEnv() + WithHost(), env-wins order so deployment identity (service.name,host.name, etc. supplied viaOTEL_RESOURCE_ATTRIBUTES) lands while runtime attrs from code (proxy.protocol,is_pro,legacy, ...) are still authoritative for the fields env doesn't set.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes