refactor(otlp-transformer): migrate to protobuf-es#6179
refactor(otlp-transformer): migrate to protobuf-es#6179overbalance merged 17 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6179 +/- ##
=======================================
Coverage 95.40% 95.40%
=======================================
Files 317 317
Lines 9440 9462 +22
Branches 2188 2199 +11
=======================================
+ Hits 9006 9027 +21
- Misses 434 435 +1
🚀 New features to boost your workflow:
|
…protobuf-es Update protobuf serializers to use fromJson + toBinary pattern with PROTOBUF_JSON_ENCODER for correct serialization. Add JSON.parse/stringify to strip undefined values that fromJson doesn't accept. Update tests to use protobuf-es APIs with toJson(alwaysEmitImplicit: true) and match protobuf JSON format expectations (string enums, string 64-bit integers, base64 bytes, all default fields).
489d33c to
315c04f
Compare
…th JSON round-trip
Add tests for empty input arrays in all three Protobuf serializers: - ProtobufTracesSerializer - ProtobufLogsSerializer - ProtobufMetricsSerializer Also clarify webpack 4 alias comment for protobuf subpath exports.
There was a problem hiding this comment.
Pull request overview
This PR migrates the otlp-transformer package from protobufjs to @bufbuild/protobuf (protobuf-es), modernizing the protobuf serialization approach and improving precision handling for 64-bit values.
Key changes:
- Replaces protobufjs with @bufbuild/protobuf 2.2.3 for generating and using protobuf definitions
- Updates serialization to use
fromJsonString+toBinarypattern with a newPROTOBUF_JSON_ENCODER - Removes precision loss in timestamp encoding by using BigInt/string representation instead of Number
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Replaces protobufjs/protobufjs-cli dependencies with @bufbuild/protobuf and related build tools |
| package-lock.json | Updates dependency tree to reflect new protobuf library; marks uglify-js as optional |
| buf.yaml | Adds Buf CLI configuration for protobuf code generation |
| buf.gen.yaml | Configures protoc-gen-es plugin to generate TypeScript protobuf code |
| src/trace/protobuf/trace.ts | Updates serialization to use protobuf-es APIs with PROTOBUF_JSON_ENCODER |
| src/metrics/protobuf/metrics.ts | Updates serialization to use protobuf-es APIs with PROTOBUF_JSON_ENCODER |
| src/logs/protobuf/logs.ts | Updates serialization to use protobuf-es APIs with PROTOBUF_JSON_ENCODER |
| src/trace/internal.ts | Adds Encoder parameter type support; removes duplicate copyright header |
| src/metrics/internal.ts | Adds Encoder parameter type support for direct encoder passing |
| src/logs/internal.ts | Adds Encoder parameter type support for direct encoder passing |
| src/common/utils.ts | Adds hexToBase64 conversion, PROTOBUF_JSON_ENCODER constant, and isOtlpEncoder type guard |
| src/common/protobuf/protobuf-export-type.ts | Removes file no longer needed with protobuf-es |
| test/utils.ts | Removes file; toBase64 functionality replaced by hexToBase64 in common/utils |
| test/trace.test.ts | Updates tests to use protobuf-es APIs and expect protobuf JSON format output |
| test/metrics.test.ts | Updates tests to use protobuf-es APIs and expect protobuf JSON format output |
| test/logs.test.ts | Updates tests to use protobuf-es APIs and expect protobuf JSON format output |
| bundler-tests/browser/webpack-4/webpack.config.mjs | Adds aliases for @bufbuild/protobuf subpath exports for Webpack 4 compatibility |
| experimental/CHANGELOG.md | Documents the refactoring in the Internal section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Response to copilot review 🤖 :
|
|
@pichlermarc If you find these changes interfere with the long term plan outlined in #5263, please let me know. |
They don't interfere. I like your PR better. It's a long-term solution. 🙂 |
pichlermarc
left a comment
There was a problem hiding this comment.
Looks great, I love PRs that remove more code than they add. 🎉
I think we can now also remove the allowJs: true from compilerOptions and
"src/generated/*.js",
"src/generated/*.ts"
from include in tsconfig.json (+ tsconfig.esm.json + tsconfig.esnext.json) as it is all generated TypeScript code now.
|
@pichlermarc Thanks! I'll make a follow up PR to simplifiy the configs. Edit: #6192 |
This reverts commit bc9e37f.
…101) ## Summary - Adds Performance Characteristics section to @connectum/otel README - Documents OTLP protobuf serializer status per signal (logs/traces/metrics) - Explains otlp-transformer version pin rationale - Provides guidance for high-volume span workloads ## Context OTel JS attempted to migrate otlp-transformer to @bufbuild/protobuf in [PR #6179](open-telemetry/opentelemetry-js#6179) but reverted in [PR #6225](open-telemetry/opentelemetry-js#6225) due to ~13x serialization slowdown (see [issue #6221](open-telemetry/opentelemetry-js#6221)). Hand-rolled ProtobufLogsSerializer landed in v0.215.0 via [PR #6390](open-telemetry/opentelemetry-js#6390); traces and metrics migrations are still pending (see [#6570](open-telemetry/opentelemetry-js#6570)). This doc makes the current state explicit for @connectum/otel consumers. ## Test plan - [x] Markdown renders correctly (biome check passes) - [x] Links to upstream issues/PRs resolve - [x] No impact on package code 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Motivation
The
protobufjslibrary useseval()in its@protobufjs/inquiredependency, which causes problems in several environments:Edge Runtimes - Cloudflare Workers, Vercel Edge, and other V8-based edge runtimes do not allow dynamic code evaluation (
eval,new Function, etc.)Chrome Extension Manifest V3 - Chrome extensions with MV3 block
eval()usage, causing blank screensContent Security Policy - Applications with strict CSP rules that prohibit
unsafe-evalcannot use protobufjsWhile PR #1941 fixed the
eval()issue in protobufjs (merged Dec 2024), the@protobufjs/inquirepackage was last published in 2017 (v1.1.0) and never received the fix.This PR migrates to @bufbuild/protobuf (protobuf-es), which:
eval()usageSee also: opentelemetry-js#4987
Summary
fromJsonString+toBinarypattern withPROTOBUF_JSON_ENCODERJSON.stringifywrapper to stripundefinedvalues that protobuf-es doesn't acceptNumber.MAX_SAFE_INTEGERTest plan