Apply W3C tracestate truncation in toHeaderString - #128
i-am-paradox wants to merge 5 commits into
Conversation
Add TraceState.toHeaderString(), which produces the tracestate header value under the W3C Trace Context 3.3.1.5 truncation procedure: entries over 128 characters are removed first, then entries are removed from the end until the joined value fits the 512-character budget. Only whole entries are dropped, and each dropped entry is reported through OTelErrorHandling. toString() stays the unbounded serializer of current state, as discussed in the issue, so a printed trace state never disagrees with what entries still holds. Fixes MindfulSoftwareLLC#126
|
Merging this after one change. I'd started drafting a decline, having re-read §3.3.1.5 and concluded that every truncation rule is gated on "needs to be truncated," so an SDK isn't obliged to truncate at all. But you built it the right way: toString() unchanged means this is opt-in at the call site, and that's exactly what's needed. The default propagator will keep calling toString() and drop nothing. A Dart server forwarding through a proxy with header limits is the caller that needs toHeaderString(), and now it has it. One addition before I merge, since the one SHOULD in that section that isn't gated is "the maximum size of the propagated tracestate header SHOULD be documented." Please sdd a paragraph to the TraceState class doc stating the policy: the 32-entry and 256-character grammar limits are enforced on every path, toString() does not truncate beyond them, and toHeaderString() applies the §3.3.1.5 procedure for callers that need a bounded header. Cite the "at least 512" sentence so nobody reads 512 as a ceiling later. Thanks for the fast turnaround and for working the open questions in the issue before building. Nice first PR! |
State the grammar limits enforced on every path, the bounded and unbounded serializers, and cite the 512 floor, per review.
|
Added the size-policy paragraph to the TraceState class doc in a8a1dc8: grammar limits on every path, the two serializers, and the 512 floor cited so it can't be read as a ceiling. |
|
Hi, thanks for tackling this topic :) And it looks good. I just have one question: Should we first check if the full header actually exceeds 512 before dropping any entries here? The spec speaks of "size limitations" and then mentions the 512.. So should we keep the full header when it fits, then remove entries over 128 one at a time while we’re over budget, followed by entries from the end if needed? We could add a test for that 134-character case, and another where removing one large entry makes the rest fit. What do you think? Or am I reading the W3C spec wrong? |
Gate the 3.3.1.5 procedure on the value actually needing truncation, per review: a value that fits 512 characters is returned as-is, even when it contains an entry over 128 characters. The removal order (over-128 first, then from the end) only applies once truncation is needed.
|
You're reading the spec right. I re-read 3.3.1.5 and the gating sentence is explicit: 'In a situation where tracestate needs to be truncated due to size limitations, the vendor MUST truncate whole entries. Entries larger than 128 characters long SHOULD be removed first.' The removal rules are part of the truncation procedure, not a standing 128-character admission limit. Reworked in 243ba9f: toHeaderString() now returns the value untouched when it fits 512, so your 134-character case keeps its big entry. Once it is over budget, the procedure runs in the order you described: over-128 entries go first (one pass, then re-join), then entries from the end until it fits. Tests updated for both: the 134-character case asserts the entry survives, and a new case shows removing one over-128 entry bringing the rest under budget without further drops. One thing I hit while reworking the fixtures: fromString/fromMap only ever hold 32 entries (the 3.3.1.1 cap), so a >512 value needs most entries to be around 13 characters, worth knowing for anyone writing cases against these limits. Full suite passes locally (1182), analyze clean. |
robert-northmind
left a comment
robert-northmind
left a comment
There was a problem hiding this comment.
Looks good. Just left one more small comment. Let me know what you think.
And should we also add a changelog entry for the new toHeaderString() method?
| if (value.length <= 512) { | ||
| return value; | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we also stop removing entries here once the remaining value fits within 512?
The initial check handles the under-budget case now 👍
But e.g. with three entries that each have a 180-character value, we start at 548 characters. Removing one brings us to 365, but the loop still removes the other two and returns an empty string.
The w3c spec doesn't explicitly say we have to check after each removal. But my reading is that once we fit the size limit, there's no need to drop more tracing data. What do you think?
Could we add a test with multiple large entries where removing just one is enough? The current cases only have one large entry, so they don't catch this. We could assert the exact entries that survive and that only one drop is reported.
| /// list-members and the per-key/per-value length rules — are enforced on | ||
| /// every path. [toString] serializes exactly what the state holds and | ||
| /// does not truncate beyond them; [toHeaderString] applies the §3.3.1.5 | ||
| /// truncation procedure for callers that need a bounded header value. | ||
| /// Vendors SHOULD propagate at least 512 characters of the combined | ||
| /// header, so 512 is a floor the procedure keeps whole entries within, |
There was a problem hiding this comment.
The over-128 loop never checks the budget, so once truncation starts it removes every over-128 entry even after the value already fits. It recomputes value on line 18 but never reads it. W3C puts over-128 entries first among the removals that are needed, not unconditionally.
for (final entry in overlong) {
if (value.length <= 512) break;
...
}Two things on cost, since this can run once per outbound request. Rejoining the whole string after every removal is O(n²); keep a running length and join once at the end. And entries.remove(entry) is a linear scan inside the loop, and MapEntry has no value equality so it only works because these are the same instances. One pass that keeps what fits avoids both.
The comment on trace_state_spec_compliance_test.dart has the smallest case that fails.
|
|
||
| test('removing one large entry can make the rest fit', () { | ||
| // 404 + 61 + 61 + separators = 528 characters: over budget. The | ||
| // over-128 entry is removed, and that alone brings the value to | ||
| // 123 characters, so the two under-128 entries both survive. | ||
| final bigValue = List.filled(400, 'v').join(); // big=... 404 chars | ||
| final ok1Value = List.filled(58, 'w').join(); // ok1=... 62 chars | ||
| final ok2Value = List.filled(58, 'x').join(); // ok2=... 62 chars | ||
| final traceState = TraceState.fromMap({ | ||
| 'ok1': ok1Value, | ||
| 'ok2': ok2Value, | ||
| 'big': bigValue, | ||
| }); | ||
| final header = traceState.toHeaderString(); | ||
| expect(header, equals('ok1=$ok1Value,ok2=$ok2Value')); | ||
| expect(header.length, lessThanOrEqualTo(512)); | ||
| }); |
There was a problem hiding this comment.
None of these tests has more than one entry over 128 characters. The loop that removes over-128 entries only goes wrong when there are two or more, so it never gets exercised.
Smallest failing case:
test('a header one character over budget keeps what fits', () {
// a=... is 255, b=... is 257, plus the comma: 513.
final ts = TraceState.fromMap({'a': 'x' * 253, 'b': 'y' * 255});
expect(ts.toHeaderString(), isNotEmpty);
});Today this returns an empty string. Both entries are over 128, so both are removed, when removing either one alone would fit.
Other cases with no coverage yet, all passing today:
- returns the value untouched at exactly 512 characters
- an entry of exactly 128 characters is not over-long
- a single maximum-size entry yields an empty header (256 key plus 256 value is 513, nothing can fit)
- truncation leaves the TraceState itself unchanged
- surviving entries keep their relative order
- reports once per dropped entry when both removal passes run
|
@robert-northmind is right, and it is worse than his example suggested. I ran his case against this branch: Three entries, 551 characters, and the method hands back an empty string. Removing one would have brought it to 367 and fit with room to spare. Detail in the review comments. Everything else here is good. The under-budget early return in 243ba9f is exactly right, and the class doc you added is the thing that makes this safe to have in the API at all. One more thing, separate from this PR. The SDK propagator does not call |
3 participants
Implements the truncation procedure from W3C Trace Context 3.3.1.5, following the shape proposed in my comment on the issue.
TraceState.toHeaderString()produces the header value the way the spec asks: entries over 128 characters are removed first, then entries are removed from the end until the joined value fits the 512-character budget. Only whole entries are dropped, and each dropped entry is reported throughOTelErrorHandlingthe same way invalid entries already are.toString()is unchanged, so it keeps serializing everything the state still holds; the unbounded and truncated forms are now distinct, and a printed state can not silently disagree withentries. If maintainers would rather have the budget configurable ortoString()itself truncate, I'm open to reshaping this.The existing 32-member cap stays where it is, in
fromStringandput: it is the separate 3.3.1.1 grammar limit.Tests cover the 128-first ordering, the end-removal ordering against the 512 boundary, whole-entry removal, the
toString/toHeaderStringsplit, and the error reports. Full suite passes locally,dart analyzeclean.With this in place, dartastic_opentelemetry#295 can proceed with the propagator delegating to this method instead of its private copy.
Fixes #126