Skip to content

Let Ruby collectors use standard OTLP headers - #4

Merged
pda merged 3 commits into
mainfrom
ruby-otlp-relay
Aug 25, 2026
Merged

Let Ruby collectors use standard OTLP headers#4
pda merged 3 commits into
mainfrom
ruby-otlp-relay

Conversation

@pda

@pda pda commented Aug 24, 2026

Copy link
Copy Markdown
Member

Why

The Ruby collector supplies its own exporter headers, which prevents the OpenTelemetry exporter from reading standard OTEL_EXPORTER_OTLP_*_HEADERS configuration. bktec's relay uses the standard trace header variable for its local credential, while otel_enabled must preserve BUILDKITE_ANALYTICS_TOKEN for normal JSON uploads.

What

Merge OTEL_EXPORTER_OTLP_TRACES_HEADERS, or its generic fallback, over collector-generated headers using case-insensitive names. Empty values are treated as unset. Standard authorization therefore takes precedence in both OpenTelemetry modes and remains authoritative across warm-worker token refreshes, without a Ruby-specific relay token variable.

Related to TE-6777.

Related PRs:

Use bktec's dedicated local relay token for OpenTelemetry exports in
both otel_enabled and otel_only modes. Keep the analytics token separate
so otel_enabled can continue authenticating its normal JSON uploads.

Document the relayed and direct-upload credential behavior and cover both
OpenTelemetry modes in the collector configuration specs.

Amp-Thread-ID: https://ampcode.com/threads/T-01a033cd-e302-7589-bcea-50c27c533600
Co-authored-by: Amp <amp@ampcode.com>
@pda
pda requested a review from a team as a code owner August 24, 2026 12:50

@buildsworth-bk-app buildsworth-bk-app Bot 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.

I didn't find an implementation issue to raise inline. This changes credential selection for OTLP exports, so it falls outside this repository's L1 approval ceiling and I'm leaving a comment rather than approving.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

Download the buildsworth logs from build 20314, then answer my questions about the findings.

Install the reading-buildsworth-logs skill to run this.

About buildsworth

Model: gpt-5.6-sol with xhigh thinking.

How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.

Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. Unless repository policy already allows L2 approval, grant it by mentioning @buildsworth-bk (see approval ceiling and L2 approval grant):

  • L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
  • L2 — Standard risk (new UI, additive API fields, refactors). Approved only when repository policy or a verified grant allows it; otherwise comment-only.
  • L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.

Merge OTEL_EXPORTER_OTLP_TRACES_HEADERS, or its generic fallback,
over the collector-generated OTLP headers using case-insensitive names.
This lets bktec's standard Bearer authorization take precedence without
replacing the analytics token used for normal JSON uploads.

Keep environment-provided authorization authoritative when a warm worker
reconfigures the collector with a refreshed analytics token. Remove the
Ruby collector's dependency on the dedicated relay token variable and
document the standard header behavior.

Amp-Thread-ID: https://ampcode.com/threads/T-01a033cd-e302-7589-bcea-50c27c533600
Co-authored-by: Amp <amp@ampcode.com>
@pda pda changed the title Let Ruby collectors use the bktec OTLP relay Let Ruby collectors use standard OTLP headers Aug 25, 2026
@pda

pda commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

@buildsworth-bk review approve L2

@pda

pda commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

🤖 Review: standard OTLP exporter header support

Verdict: yes, this correctly adds support for standard OTel exporter headers. The behavior was verified against both the OTel exporter spec and the reference implementation in opentelemetry-ruby's OTLP exporter, and all 45 examples in otel_spec.rb pass at this head.

What matches the standard

  • Precedence: OTEL_EXPORTER_OTLP_TRACES_HEADERS first, falling back to OTEL_EXPORTER_OTLP_HEADERS only when the signal-specific var is nil, with no merging between the two. This is exactly the SDK's config_opt (first non-nil) semantics and matches the spec's signal-specific-takes-precedence rule. The first new test correctly asserts the generic var is ignored entirely when the trace-specific one is set.
  • Parsing: otlp_headers_from_environment is a near byte-for-byte match of the SDK's parse_headers — comma-separated entries, split on the first = (values may contain =), URI.decode_uri_component on both key and value (correctly not CGI.unescape, so + is not form-decoded), strip, and ArgumentError on empty input or empty key/value.
  • Why re-implementation is needed at all: confirmed — passing explicit headers: to the Ruby OTLP exporter constructor bypasses its env-var defaults entirely, so merging in the collector is the only way to honor the standard vars. The comment in configure! states this accurately.
  • Failure mode: an invalid header string raises out of otlp_headers_from_environment into configure!'s existing rescue LoadError, StandardError, which warns and disables export. Tests never break — consistent with the collector's fail-open design.

Sensible judgment calls (fine as-is)

  • Case-insensitive merge: env headers replace collector headers via casecmp?, so a standard lowercase authorization correctly displaces the collector's Authorization. The spec doesn't mandate this (the SDK never merges, so it has no such problem), but HTTP header names are case-insensitive, so this is the right call for a merge.
  • Warm-worker refresh guard: @authorization_from_environment is only set when the env headers actually contain an authorization key (any case), keeping standard config authoritative across refresh_authorization without blocking refresh when env vars only carry non-auth headers. It's cleared in shutdown. The new refresh test covers the double-configure! path.
  • User-Agent is unaffected: headers are passed as a Hash, and the exporter's prepare_headers still appends its own User-Agent.

One deviation worth knowing about (not blocking)

The OTel spec says an empty env var value SHOULD be treated as unset, but here OTEL_EXPORTER_OTLP_TRACES_HEADERS="" raises (empty splitArgumentError) and disables export instead of falling back to the generic var / collector headers. However, this exactly mirrors opentelemetry-ruby's own fail-fast behavior on empty strings, so matching the reference implementation for the ecosystem is a defensible — arguably preferable — choice. The consequence is just gentler here: export is disabled with a warning rather than the exporter raising.


Review by Amp (agentic coding tool) using the Fable 5 model — thread: https://ampcode.com/threads/T-01a03641-aa42-713f-b8e4-5b130435ae87

@pda

pda commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

The OTel spec says an empty env var value SHOULD be treated as unset, but here OTEL_EXPORTER_OTLP_TRACES_HEADERS="" raises

Fixing…

Skip empty trace-specific and generic OTLP header environment values in
accordance with the OpenTelemetry configuration specification. An empty
trace-specific value now falls back to the generic setting, and two empty
values leave the collector's normal run-key and authorization headers in
place instead of disabling OpenTelemetry export.

Amp-Thread-ID: https://ampcode.com/threads/T-01a033cd-e302-7589-bcea-50c27c533600
Co-authored-by: Amp <amp@ampcode.com>
@pda
pda merged commit 855708e into main Aug 25, 2026
4 checks passed
@pda
pda deleted the ruby-otlp-relay branch August 25, 2026 01:02
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.

2 participants