Skip to content

feat: [#726] Harden OTLP exporter configuration [7] - #1475

Merged
krishankumar01 merged 14 commits into
masterfrom
kkumar-gcc/#726-otlp-exporter-hardening
Jun 9, 2026
Merged

feat: [#726] Harden OTLP exporter configuration [7]#1475
krishankumar01 merged 14 commits into
masterfrom
kkumar-gcc/#726-otlp-exporter-hardening

Conversation

@krishankumar01

@krishankumar01 krishankumar01 commented Jun 5, 2026

Copy link
Copy Markdown
Member

📑 Description

RelatedTo goravel/goravel#726

The OTLP exporter config advertises a protocol the SDK cannot do, exports wrong data for UpDownCounters under delta temporality, and lacks TLS, compression, retry tuning, and URL-path endpoints. Telemetry is unreleased, so these correctness fixes land before the v1.18 config shape freezes.

What is changing (with examples):

  • metric_temporality delegates to the SDK preset selectors instead of a hand-rolled switch; lowmemory added as the third standard value.
"metric_temporality": "delta"

inFlight, _ := meter.Int64UpDownCounter("jobs.in_flight")
inFlight.Add(ctx, 5)
inFlight.Add(ctx, -3)
// before: exported as deltas (+5, -3), level unrecoverable on the backend
// after: exported cumulative (2), per the OTLP spec; Counters/Histograms still delta
  • http/json removed; unknown protocols fail at boot instead of silently exporting protobuf.
"protocol": "http/json"
// before: silently exports http/protobuf
// after: boot error "unsupported telemetry exporter protocol: http/json"
  • Endpoints with a scheme go through WithEndpointURL, so paths survive and security derives from the scheme.
"endpoint": "https://collector.example.com/otel"
// before: scheme stripped, path mangled, every export 404s
// after: exports to https://collector.example.com/otel/v1/traces
  • tls block (ca/cert/key file paths) for private-CA and mTLS collectors; conflicts with insecure are rejected at boot.
"tls": map[string]any{
    "ca":   "/etc/ssl/private-ca.pem",
    "cert": "/etc/ssl/client.pem", // optional pair for mTLS
    "key":  "/etc/ssl/client.key",
},
  • compression ("gzip") and retry tuning; retry stays SDK-default (enabled, 5s/30s/1m) when omitted.
"compression": "gzip",
"retry": map[string]any{
    "enabled":          true,
    "max_elapsed_time": "10s", // short-lived processes: stop retrying before shutdown_timeout
},
  • processor block for traces and logs: "batch" (default) or "simple" (synchronous export, useful with the console exporter during development), plus batch interval/timeout; unknown types fail at boot.
"processor": map[string]any{
    "type":     "batch",
    "interval": "5s",
    "timeout":  "30s",
},

Side effects / breaking changes:

  • None for users: telemetry is unreleased. ProtocolHTTPJSON and the old all-delta selector are removed pre-release.

✅ Checks

  • Added test cases for my code

@codecov

codecov Bot commented Jun 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 78.62319% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.43%. Comparing base (ee628ab) to head (c4eb084).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
telemetry/setup/stubs.go 0.00% 45 Missing ⚠️
telemetry/metric.go 91.66% 2 Missing and 2 partials ⚠️
telemetry/otlp.go 91.83% 2 Missing and 2 partials ⚠️
telemetry/config.go 80.00% 1 Missing and 1 partial ⚠️
telemetry/log.go 96.77% 1 Missing and 1 partial ⚠️
telemetry/trace.go 96.77% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #1475    +/-   ##
========================================
  Coverage   69.43%   69.43%            
========================================
  Files         375      376     +1     
  Lines       29730    30097   +367     
========================================
+ Hits        20642    20899   +257     
- Misses       8141     8231    +90     
- Partials      947      967    +20     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@krishankumar01
krishankumar01 marked this pull request as ready for review June 6, 2026 19:51
@krishankumar01
krishankumar01 requested a review from a team as a code owner June 6, 2026 19:51
Copilot AI review requested due to automatic review settings June 6, 2026 19:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the OTLP telemetry exporter configuration by aligning protocol/temporality behavior with the OpenTelemetry SDK capabilities and adding missing transport features (URL-path endpoints, TLS, compression, retry), while failing fast on unsupported configuration values.

Changes:

  • Make OTLP protocol handling strict (unsupported protocols error at boot) and add endpoint URL-path support for OTLP/HTTP exporters.
  • Add OTLP exporter options for TLS (CA/mTLS), compression (gzip), and retry tuning for traces/metrics/logs.
  • Delegate metric temporality selection to SDK selectors (including a new lowmemory preset) and extend test coverage accordingly.

Reviewed changes

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

Show a summary per file
File Description
telemetry/trace.go Refactors OTLP trace exporter option building; adds compression/TLS/retry and strict protocol handling.
telemetry/trace_test.go Adds config-level and exporter-level tests for unsupported protocol, URL paths, compression/retry, and TLS conflicts.
telemetry/setup/stubs.go Updates telemetry config stubs: removes http/json, documents temporality presets, adds compression/tls/retry blocks.
telemetry/otlp.go Introduces shared OTLP option builder, endpoint parsing (scheme/path), TLS config loader, compression and retry support.
telemetry/otlp_test.go Adds unit tests for endpoint parsing, compression/retry, and TLS config behavior.
telemetry/metric.go Updates OTLP metric exporter to use shared option builder; adds compression/TLS/retry and SDK temporality selectors.
telemetry/metric_test.go Extends tests for strict protocol errors and temporality selector behavior across instrument kinds.
telemetry/log.go Updates OTLP log exporter to use shared option builder; adds compression/TLS/retry and strict protocol handling.
telemetry/log_test.go Adds tests for unsupported protocol, URL paths, compression/retry, and TLS conflicts for log exporter setup.
telemetry/config.go Extends exporter config schema with compression, TLS, and retry settings.
errors/list.go Adds centralized telemetry errors for unsupported protocol/compression and TLS-related config validation.

Comment thread telemetry/otlp.go
Comment thread telemetry/config.go
Copilot AI review requested due to automatic review settings June 7, 2026 08:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Comment thread telemetry/otlp.go Outdated
Comment thread telemetry/setup/stubs.go
Comment on lines +233 to +237
"tls": map[string]any{
"ca": config.Env("OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE", ""),
"cert": config.Env("OTEL_EXPORTER_OTLP_TRACES_CLIENT_CERTIFICATE", ""),
"key": config.Env("OTEL_EXPORTER_OTLP_TRACES_CLIENT_KEY", ""),
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to test the tls feature in goravel/example?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will raise a PR for this in goravel/example

Comment thread telemetry/otlp.go Outdated
Copilot AI review requested due to automatic review settings June 8, 2026 15:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Comment thread telemetry/trace.go
Comment thread telemetry/log.go
Comment thread telemetry/log.go
Comment thread telemetry/trace_test.go
Comment thread telemetry/trace_test.go
Comment thread telemetry/log_test.go
Comment thread telemetry/log_test.go
Comment thread telemetry/otlp.go Outdated
Comment on lines 51 to 53
if usesInsecureTransport(cfg) {
opts = append(opts, builders.withInsecure())
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relationship between line 51 and line 39 should be if else, right? The current code structure is a bit strange. Could you optimize it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Earlier we were parsing the url and setting insecure manually. That was only to work around the otlploghttp bug where WithEndpointURL does not add /v1/logs when the url has no path (trace and metric handle it, logs do not).

Now we let the SDK do it. WithEndpointURL for url endpoints, and WithEndpoint with the insecure flag for bare host. So usesInsecureTransport and the manual parsing are removed.

One known thing, a scheme log endpoint without a path will still miss /v1/logs because of the same SDK bug. So we keep the default endpoint as bare localhost:4318.

Use WithEndpointURL for scheme-bearing endpoints (host, path, and insecure
derive from the URL) and WithEndpoint plus the insecure flag for bare hosts,
instead of hand-parsing the URL and re-deriving the scheme. Drops the redundant
usesInsecureTransport helper, the withURLPath wiring, and the
TLS-conflicts-with-insecure error -- TLS-vs-insecure precedence is the SDK's
job. Defaults the stub endpoints to bare localhost:4318 so logs are unaffected
by otlploghttp's empty-path defect.
Copilot AI review requested due to automatic review settings June 9, 2026 06:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Comment thread telemetry/otlp.go

@hwbrzzl hwbrzzl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@hwbrzzl

hwbrzzl commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

The example ci should be fixed in the master.

@krishankumar01

Copy link
Copy Markdown
Member Author

The example ci should be fixed in the master.

I will fix it in currently raised PR in example repo

@krishankumar01
krishankumar01 merged commit 56ea61c into master Jun 9, 2026
17 of 21 checks passed
@krishankumar01
krishankumar01 deleted the kkumar-gcc/#726-otlp-exporter-hardening branch June 9, 2026 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants