fix: use otel sdk to parse headers - #1376
Conversation
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
0b77577 to
5bde7eb
Compare
ferhatelmas
changed the title
Coverage Report for CI Build 34385423542Coverage increased (+0.09%) to 82.461%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — straightforward deletion of duplicated/incomplete custom OTLP header parsing in favor of the SDK's built-in env-var handling, with a new test file (otel-exporter-headers.test.ts) that verifies percent-decoding, duplicate-key precedence, and malformed-entry tolerance against the real exporter classes.
Reviewed: removal of the manual header-parsing/grpc.Metadata construction in otel-metrics.ts and otel-tracing.ts, and the corresponding test updates in otel-metrics.test.ts/otel-tracing.test.ts. Confirmed no other callers relied on the removed exporterHeaders/grpcMetadata exports (they were module-local consts, not exported). Also confirmed the previous trace exporter never passed the plain headers option (only metadata), so removing both is consistent with the metrics exporter's prior behavior.
Extended reasoning...
Overview
This PR removes hand-rolled OTLP header parsing (splitting OTEL_EXPORTER_OTLP_{METRICS,TRACES}_HEADERS on ,/= and building a grpc.Metadata object) from otel-metrics.ts and otel-tracing.ts, deleting the now-unused @ grpc/grpc-js imports in those files. It relies instead on the OTLP gRPC exporter packages' built-in env-var parsing (which correctly percent-decodes and handles the common+signal-specific header merge). A new shared test file, otel-exporter-headers.test.ts, instantiates the real OTLPMetricExporter/OTLPTraceExporter classes and spies on grpc.Metadata.prototype.set to assert the library parses headers correctly (percent-decoding, last-value-wins on duplicate keys, tolerance of malformed/empty entries). The old tests that asserted the manual metadata construction are removed/updated accordingly.
Security risks
None of significance. This only affects how OTLP exporter authentication/metadata headers are parsed from environment variables at process startup; it does not change trust boundaries, does not introduce new external input parsing beyond what the well-maintained OpenTelemetry SDK already does, and removes code rather than adding new attack surface.
Level of scrutiny
This is a small, mechanical refactor confined to telemetry/monitoring initialization code (not auth, crypto, or request-handling paths), with net negative code and a dedicated new test file directly exercising the replaced behavior against the real exporter classes. That warrants a light level of scrutiny.
Other factors
A previous investigation (recorded in the ruled-out list) noted a version-resolution nuance (exporter-trace-otlp-grpc resolving to 0.220.0 vs exporter-metrics-otlp-grpc at 0.221.0) but found no issue; I did not find anything to add beyond that. The diff is fully covered by the new/updated tests, and the change directly addresses the linked bug (incomplete percent-decoding/wrong split in the custom parser).
This review covers commit 0b77577, which is no longer the latest commit on this pull request; later commits are not covered by it.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
We do custom parsing for otel headers but it isn't complete (percent decoding, wrong split #1375). We also duplicate it for metrics and traces.
What is the new behavior?
Drop custom parser and override so that SDK can do it automatically.
Additional context
Related to #1375