Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions lib/src/api/trace/trace_state.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ part 'trace_state_create.dart';

/// Key-value pairs carried along with a span context.
/// TraceState follows the W3C Trace Context specification.
///
/// Size policy: the grammar limits (W3C §3.3.1.1) — a maximum of 32
/// 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,
Comment on lines +14 to +19

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// not a ceiling imposed on the state itself.
class TraceState {
static const int _maxKeyValuePairs = 32;
static final RegExp _simpleKeyFormat = RegExp(r'^[a-z][a-z0-9_\-*/]{0,255}$');
Expand Down Expand Up @@ -119,6 +128,48 @@ class TraceState {
return _entries.entries.map((e) => '${e.key}=${e.value}').join(',');
}

/// Produces the W3C `tracestate` header value, applying the truncation
/// procedure of W3C Trace Context §3.3.1.5.
///
/// The procedure only runs when the value needs to be truncated: if the
/// joined value fits the 512-character budget it is returned as-is,
/// including entries over 128 characters. When it does not fit, whole
/// entries are removed, entries larger than 128 characters first, then
/// entries from the end until the value fits. Every dropped entry is
/// reported through [OTelErrorHandling]. Unlike [toString], this may
/// return a value that no longer contains all entries.
String toHeaderString() {
var value = _entries.entries.map((e) => '${e.key}=${e.value}').join(',');
if (value.length <= 512) {
return value;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// W3C §3.3.1.5: "Entries larger than 128 characters long SHOULD be
// removed first", as part of truncating a value that does not fit.
// The length of a list-member is its `key=value` size.
final entries = List<MapEntry<String, String>>.from(_entries.entries);
final overlong = entries
.where((e) => '${e.key}=${e.value}'.length > 128)
.toList(growable: false);
for (final entry in overlong) {
entries.remove(entry);
OTelErrorHandling.report(StateError(
'TraceState entry ${entry.key} exceeds 128 characters; dropped.'));
value = entries.map((e) => '${e.key}=${e.value}').join(',');
}

// Then entries should be removed starting from the end of the
// tracestate until the value fits the 512-character budget.
while (value.length > 512 && entries.isNotEmpty) {
final entry = entries.removeLast();
OTelErrorHandling.report(StateError(
'TraceState exceeds 512 characters; entry ${entry.key} dropped.'));
value = entries.map((e) => '${e.key}=${e.value}').join(',');
}

return value;
}

/// Validate a tracestate key: a simple key, or a multi-tenant
/// `tenant-id@system-id` key.
static bool _isValidKey(String key) {
Expand Down
120 changes: 120 additions & 0 deletions test/unit/api/trace/trace_state_spec_compliance_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -101,4 +101,124 @@ void main() {
expect(result.get('vendor'), isNull);
});
});

group('TraceState toHeaderString follows W3C 3.3.1.5 truncation', () {
test('returns the full value when it fits the budget', () {
final traceState = TraceState.fromMap({'a': '1', 'b': '2'});
expect(traceState.toHeaderString(), equals('a=1,b=2'));
});

test('returns empty string for an empty state', () {
final traceState = TraceState.empty();
expect(traceState.toHeaderString(), equals(''));
});

test('keeps an over-128 entry when the total fits 512', () {
// Robert's case: 134 characters total, under budget. The 128 rule
// only applies once truncation is needed (W3C 3.3.1.5), so the
// entry stays.
final longValue = List.filled(130, 'v').join();
final traceState = TraceState.fromMap({'big': longValue});
final header = traceState.toHeaderString();
expect(header, equals('big=$longValue'));
});

test('removes over-128 entries first once truncation is needed', () {
// 32 entries survive the grammar cap: the 134-character entry
// plus 31 twelve-character ones total 567 characters, over budget.
// The over-128 entry is removed first, and that alone brings the
// value to 433 characters, so the rest survive.
final bigValue = List.filled(130, 'v').join(); // big=... 134 chars
final entries = <String, String>{'big': bigValue};
for (var i = 0; i < 31; i++) {
entries['a$i'] = 'y' * 10; // a0=..., 13 chars each with separator
}
final traceState = TraceState.fromMap(entries);
expect(traceState.entries.length, 32);
final header = traceState.toHeaderString();
expect(header.contains('big'), isFalse,
reason: 'over-128 entries are removed first');
expect(header.length, lessThanOrEqualTo(512));
});

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));
});
Comment on lines +143 to +159

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


test('keeps a long-but-under-128 entry when shorter entries go first', () {
// second's entry is 120 characters (key 6 + '=' 1 + value 113),
// under the 128 limit, while first is tiny. Nothing is dropped here,
// the point is the size does not trigger the 128-char removal.
final longValue = List.filled(113, 'x').join();
final traceState = TraceState.fromMap({
'first': 'old',
'second': longValue,
});
final header = traceState.toHeaderString();
expect(header, equals('first=old,second=$longValue'));
});

test('removes whole entries from the end to fit 512 characters', () {
final entries = <String, String>{};
// 10 entries of ~60 characters each: 600+ total, must drop some.
final v55 = List.filled(55, 'v').join();
for (var i = 0; i < 10; i++) {
entries['k$i'] = v55;
}
final traceState = TraceState.fromMap(entries);
final header = traceState.toHeaderString();
expect(header.length, lessThanOrEqualTo(512));
// Whole entries only: the header still ends on a complete entry.
expect(header.endsWith('}'), isFalse); // sanity, no partial values
expect(header.contains('k0='), isTrue, reason: 'oldest kept');
// Some of the newest entries had to go.
expect(header.contains('k9='), isFalse);
});

test('toString keeps all entries when toHeaderString truncates', () {
final entries = <String, String>{};
final v55 = List.filled(55, 'v').join();
for (var i = 0; i < 10; i++) {
entries['k$i'] = v55;
}
final traceState = TraceState.fromMap(entries);
final header = traceState.toHeaderString();
expect(header.length, lessThanOrEqualTo(512));
expect(traceState.toString().length, greaterThan(512));
});

test('reports dropped entries through OTelErrorHandling', () {
final received = <Object>[];
OTelErrorHandling.handler = (error, stackTrace) {
received.add(error);
};
try {
final longValue = List.filled(200, 'v').join();
final entries = <String, String>{'big': longValue};
for (var i = 0; i < 31; i++) {
entries['a$i'] = 'y' * 10;
}
final traceState = TraceState.fromMap(entries);
traceState.toHeaderString();
expect(received.length, 1,
reason: 'the overlong entry must be reported once');
expect(received.single.toString(), contains('big'));
} finally {
OTelErrorHandling.resetToDefault();
}
});
});
}