feat(otlp-transformer): add custom logs protobuf serializer#6390
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6390 +/- ##
==========================================
- Coverage 95.75% 95.71% -0.04%
==========================================
Files 364 369 +5
Lines 12095 12461 +366
Branches 2884 2945 +61
==========================================
+ Hits 11581 11927 +346
- Misses 514 534 +20
🚀 New features to boost your workflow:
|
3a90e81 to
bef2eb7
Compare
|
Future enhancements not for this PR
|
|
thanks @dyladan for the review. I'll go over the remaining comments on monday :) avoiding the string operations for preorganizing the log records is a good call. In practice we do always put the same object on there so it can be used as a key. I changed the comment in sdk-logs to reflect that. people may construct |
05024f7 to
e5511af
Compare
| import type { IExportLogsServiceRequest } from '../internal-types'; | ||
| import type { IExportLogsServiceResponse } from '../export-response'; | ||
|
|
||
| import { createExportLogsServiceRequest } from '../internal'; |
There was a problem hiding this comment.
Should we @deprecate or even fully remove this function now?
There was a problem hiding this comment.
We still use it during JSON serialization so we still need to keep it around for now.
With this PR some opportunity of improving the JSON path open up since we don't need to support protobuf on the same code-path anymore (removing Encoder comes to mind, it's primary purpose is to facilitate protobuf and JSON on the same code-path), but I'd keep this PR scoped to just protobuf for now.
It'll also be a lot easier to do once we've migrated all signals to a hand-rolled proto implementation; all signals share some common code that can be simplified at once.
## Summary Quarterly OpenTelemetry dependency bump: `@opentelemetry/api-logs` and friends `0.212.0 → 0.215.0`, stable packages (`resources`, `sdk-metrics`, `sdk-trace-node`) `2.5.1 → 2.7.0`, `semantic-conventions` `1.39.0 → 1.40.0`. ## Breaking change impact check Two upstream breaking changes were analyzed against `packages/otel/src/`: 1. **Custom `LogRecordExporter.forceFlush()` now required** — N/A. `@connectum/otel` uses only stock exporters (`OTLPLogExporterHTTP`, `OTLPLogExporterGRPC`, `ConsoleLogRecordExporter`); no `implements LogRecordExporter` anywhere in source. 2. **gRPC exporter config `headers` field removed** — N/A. The internal `CollectorOptions` interface has only `concurrencyLimit` and `url`; no `headers` is passed into the gRPC exporter constructors. ## Feature auto-gains (no API changes) - Hand-rolled `ProtobufLogsSerializer` (PR open-telemetry/opentelemetry-js#6390, v0.215.0) — ~43% throughput improvement for logs protobuf serialization. - `cardinalitySelector` option in `PeriodicExportingMetricReader` (PR #6460, v2.7.0) — protects against label cardinality explosion (e.g. per `rpc.method`). Can be wired in a follow-up. - SDK self-monitoring metrics: span creation (PR #6213, v2.6.0) and log creation (PR #6433, v2.7.0). - Prototype pollution safety patch in `mergeTwoObjects` (PR #6587, v2.7.0). - Stable RPC semantic conventions from semconv 1.28–1.30 (`rpc.response.status_code`, `error.type`). ## Quality gates - L2 build: 16/16 successful - L2 typecheck: clean (`tsc --noEmit`) - L2 test: 29/29 successful, 139 tests in `@connectum/events` alone, 0 failures - L3 lint (biome): 13/13, no fixes applied ## Changeset Patch bump for `@connectum/otel` (no public API changes, only underlying SDK version). ## Test plan - [x] `pnpm install` regenerates lockfile cleanly - [x] `pnpm build && pnpm typecheck && pnpm test` pass - [x] `pnpm lint` passes - [ ] CI green on PR - [ ] Smoke test `performance-test-server` example post-merge (cardinality/throughput benchmark) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated OpenTelemetry SDK and related packages to the latest stable versions, including upstream bug fixes and feature improvements. * Updated semantic conventions to the latest version for improved standards compliance. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…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)
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `main` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `main`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @connectum/auth@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/core@1.0.0-rc.11 ## @connectum/events@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/core@1.0.0-rc.11 ## @connectum/events-amqp@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/events@1.0.0-rc.11 ## @connectum/events-kafka@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/events@1.0.0-rc.11 ## @connectum/events-nats@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/events@1.0.0-rc.11 ## @connectum/events-redis@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/events@1.0.0-rc.11 ## @connectum/healthcheck@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/core@1.0.0-rc.11 ## @connectum/interceptors@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/core@1.0.0-rc.11 ## @connectum/otel@1.0.0-rc.11 ### Patch Changes - [#98](#98) [`15f4dbb`](15f4dbb) Thanks [@intech](https://github.com/intech)! - Bump OpenTelemetry SDK to 0.215.0 / v2.7.0 and semantic conventions to 1.40.0. Highlights (auto-gain, no API changes in `@connectum/otel`): - Hand-rolled `ProtobufLogsSerializer` (PR open-telemetry/opentelemetry-js#6390, v0.215.0) — +67–73% throughput for typical batch sizes (100–1024 logs); +72% at 512 logs, +67% at 1024 logs per upstream benchmarks in PR [#6228](https://github.com/Connectum-Framework/connectum/issues/6228) - `cardinalitySelector` support in `PeriodicExportingMetricReader` (PR [#6460](https://github.com/Connectum-Framework/connectum/issues/6460), v2.7.0) — protection against cardinality explosion on high-variance attributes - SDK self-observability: span + log creation metrics (PRs [#6213](https://github.com/Connectum-Framework/connectum/issues/6213), [#6433](https://github.com/Connectum-Framework/connectum/issues/6433)) - Internal `mergeTwoObjects` safety checks (PR [#6587](https://github.com/Connectum-Framework/connectum/issues/6587), v2.7.0) — additional guards against unsafe key merges - Updated semantic conventions (semconv v1.40.0) — stable RPC attributes including `rpc.response.status_code` and `error.type` (stabilized in semconv v1.39.0) Breaking changes upstream that do NOT affect `@connectum/otel` (verified): - Custom `LogRecordExporter.forceFlush()` requirement — not applicable (we use stock exporters only) - gRPC exporter config `headers` field removal — not applicable (`CollectorOptions` has no `headers`) - [#99](#99) [`5b3f01d`](5b3f01d) Thanks [@intech](https://github.com/intech)! - security(deps): force patched versions of protobufjs and basic-ftp via pnpm overrides Resolves Dependabot alerts on main branch: - **GHSA-xq3m-2v4x-88gg** (Critical) — Arbitrary code execution in protobufjs < 7.5.5 (transitive via `@grpc/proto-loader` under OTel gRPC exporters). - **GHSA-xq3m-2v4x-88gg** (Critical) — Arbitrary code execution in protobufjs 8.0.0 (transitive via `@opentelemetry/otlp-transformer`). - **GHSA-chqc-8p9q-pq6q** (High) — basic-ftp 5.2.0 FTP Command Injection via CRLF (dev-only transitive via `@exodus/test` → puppeteer-core). - **GHSA-6v7q-wjvx-w8wg** (High) — basic-ftp ≤ 5.2.1 incomplete CRLF protection (dev-only transitive via `@exodus/test` → puppeteer-core). No runtime API changes. Only `pnpm.overrides` in the monorepo root were adjusted to force patched transitive versions: `protobufjs@<7.5.5 → 7.5.5`, `protobufjs@>=8.0.0 <8.0.1 → 8.0.1`, `basic-ftp@<5.2.2 → 5.2.2`. ## @connectum/reflection@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/core@1.0.0-rc.11 ## @connectum/testing@1.0.0-rc.11 ### Patch Changes - Updated dependencies \[]: - @connectum/core@1.0.0-rc.11 ## @connectum/cli@1.0.0-rc.11 ## @connectum/core@1.0.0-rc.11 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adds toBinaryFast() — opt-in fast path using two-pass size estimation and a pre-allocated buffer for write. Ports the pattern from open-telemetry/opentelemetry-js#6390 (ProtobufLogsSerializer) to the protobuf-es reflective encode. Motivation ---------- The existing toBinary uses BinaryWriter with fork/join per length- delimited field — every nested message and every packed repeated scalar pushes chunk/buf state onto a stack, serializes into its own chunk list, then re-emits a varint length prefix and concatenates. On OTel-shaped workloads (ResourceSpans -> ScopeSpans -> Span -> KeyValue) that produces many small Uint8Array/number[] allocations and a final double-copy in finish(). The two-pass variant walks the descriptor once to compute the exact encoded size, allocates a single Uint8Array of that size, then writes bytes into it at fixed offsets. Length prefixes computed in pass 1 are cached per submessage object and reused in pass 2, so pass 2 is a straight-line write loop. Results (Node 25.8.1, x86_64, tinybench, OTel-like 100-span payload) -------------------------------------------------------------------- create() + toBinary() combined workload: create + toBinary 353 ops/s baseline create + toBinaryFast 1758 ops/s +397% (4.98x) toBinary() on pre-built message: toBinary 385 ops/s baseline toBinaryFast 2417 ops/s +528% (6.28x) Cross-library (vs protobufjs pbjs static-module): protobuf-es toBinary pre-built 428 ops/s protobuf-es toBinaryFast pre-built 3868 ops/s protobufjs encode pre-built 3259 ops/s -> toBinaryFast beats protobufjs by +19% on encode path. Memory (1000 iters, forced GC, heapUsed delta): protobuf-es create+toBinary 10,211 B/op protobuf-es create+toBinaryFast 4,670 B/op -54% protobufjs create+encode 7,450 B/op -> toBinaryFast now uses less heap than protobufjs. Scope (MVP) ----------- Supported: all 15 scalar types, enum, repeated scalar (packed and unpacked), nested messages, repeated messages. Correctness verified with semantic round-trip (decode(toBinaryFast) structurally-equal to decode(toBinary)) on the OTel ExportTraceRequest fixture and on SimpleMessage; both fixtures in fact produce byte-identical output in the current code path. Fallback: schemas using maps, oneofs, extensions, or delimited/group encoding fall back to toBinary. The decision is cached per DescMessage in a WeakMap, so the support check does not dominate the hot path after the first call. Unknown fields are dropped by the fast path. Callers that must round-trip unknown fields should continue to use toBinary. Testing ------- - Existing protobuf-test suite: 2823/2823 passing. - Correctness verification: benchmarks/src/verify-correctness.ts exercises ExportTraceRequest and SimpleMessage fixtures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Which problem is this PR solving?
Currently we run into warnings that CSPs are being violated by
protobuf.js(#4987). Also, to convert toprotobuf.js' format, we do have to go through an intermediate representation, which is costly in terms of allocations.This PR moves toward fixing this by introducing a custom protobuf serializer that can be used instead.
Note: my previous PR #6228 had a simple dynamic growing approach. However, since there is a risk that memory behavior will change for the worse from the current
protobuf.js-based implementation, which does a double-pass to determine the size of the final message, I opted to also implement a double-pass approach. The first pass determines the buffer size and the second pass actually writes to said buffer. This makes the approach slightly slower than what I proposed in #6228 (also slower than JSON serialization, but faster than theprotobuf.js-based approach), but more predictable than the doubling the buffer size or always allocating 64KB, which may not be needed.Towards fixing:
Part of:
Supersedes #6228
Relevant benchmark results:
Old:
New:
Disclosure of AI use: I generated the initial prototype of this with GitHub Copilot and Claude Sonnet 4.5, but then applied a whole bunch of optimizations and changes to it to make it fit what we're trying to do here.
Short description of the changes
Type of change
How Has This Been Tested?