[otap-df-pdata/common] variable size protobuf placeholder#2589
Merged
jmacd merged 4 commits intoopen-telemetry:mainfrom Apr 9, 2026
Merged
[otap-df-pdata/common] variable size protobuf placeholder#2589jmacd merged 4 commits intoopen-telemetry:mainfrom
jmacd merged 4 commits intoopen-telemetry:mainfrom
Conversation
Add an optional fourth argument to proto_encode_len_delimited_unknown_size! macro to specify the number of bytes (1-4) for the length placeholder varint. The existing 3-argument form continues to default to 4 bytes. Use const generics for encode_len_placeholder<N> and patch_len_placeholder<N> so the placeholder size is known at compile time, enabling the compiler to inline fixed-size arrays and unroll patch loops. Invalid sizes (outside 1-4) are caught at compile time via a const assertion. Add named convenience macros: - proto_encode_len_delimited_small! (2-byte, up to 16 KB) - proto_encode_len_delimited_large! (4-byte, up to 256 MB) Update all 24 macro callsites in the telemetry crate's self_tracing encoder (128-4096 bytes), well within the 16 KB ceiling of a 2-byte varint, saving 2 bytes of overhead per nesting level. The pdata crate's OTAP encoding callsites (~40) continue using the default 4-byte placeholders since they encode full OTLP requests that can be large. Part of open-telemetry#1746 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2589 +/- ##
=======================================
Coverage 88.37% 88.38%
=======================================
Files 622 622
Lines 230058 230115 +57
=======================================
+ Hits 203318 203382 +64
+ Misses 26216 26209 -7
Partials 524 524
🚀 New features to boost your workflow:
|
albertlockett
approved these changes
Apr 8, 2026
Member
albertlockett
left a comment
albertlockett
left a comment
There was a problem hiding this comment.
LGTM - assuming you'll fix the clippies
As much as I like the mysterious nature of the name proto_encode_len_delimited_unknown_size, I'll concede that proto_encode_len_delimited_of_size seems much more respectable
jmacd
force-pushed
the
jmacd/configurable-placeholder-size
branch
from
b65b1ec to
392e83d
Compare
…d/configurable-placeholder-size
Contributor
Author
|
To avoid a large blast radius, I left the old |
Merged
via the queue into
open-telemetry:main
with commit Apr 9, 2026
4a8d85d
68 of 70 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change Summary
Add an optional fourth argument to proto_encode_len_delimited_of_size! macro to specify the number of bytes for the length placeholder. The existing 3-argument form continues to default to 4 bytes.
Update all callsites in the telemetry crate's self_tracing encoder to use 2-byte placeholders.
Part of #1746
How are these changes tested?
✅
Are there any user-facing changes?
No