Skip to content

Commit a28f12f

Browse files
david-lunapichlermarc
david-luna
and
pichlermarc
authored
fix(opentelemetry-core): defer tracestate vaidation (#6459)
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
1 parent b27c514 commit a28f12f

5 files changed

Lines changed: 220 additions & 74 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ For notes on migrating to 2.x / 0.200.x see [the upgrade guide](doc/upgrade-to-2
1616

1717
### :bug: Bug Fixes
1818

19+
* fix(core, api): defer trace state validation. Deprecate trace state implementation in api [#6459](https://github.com/open-telemetry/opentelemetry-js/pull/6459) @david-luna
20+
* **important:** this bug fix may be breaking for certain uses of `TraceState`
21+
* `set` now returns the same `TraceState` instance if key/value are invalid or makes the while trace state invalid.
22+
* `unset` now returns the same `TraceState` instance if key is not present.
23+
* best-effort parsing of invalid `TraceState`s has changed: when multiple keys with the same name are present, the most recent one will win.
24+
1925
### :books: Documentation
2026

2127
### :house: Internal

‎api/src/trace/internal/tracestate-impl.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ const LIST_MEMBER_KEY_VALUE_SPLITTER = '=';
1919
* - New key-value pair should be added into the beginning of the list
2020
* - The value of any key can be updated. Modified keys MUST be moved to the
2121
* beginning of the list.
22+
*
23+
* @deprecated Use TraceState from "@opentelemetry/core". This will be removed in the next major version.
2224
*/
2325
export class TraceStateImpl implements TraceState {
2426
private _internalState: Map<string, string> = new Map();

‎api/src/trace/internal/utils.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import type { TraceState } from '../trace_state';
77
import { TraceStateImpl } from './tracestate-impl';
88

99
/**
10+
* @deprecated Use TraceState from "@opentelemetry/core". This will be removed in the next major version.
1011
* @since 1.1.0
1112
*/
1213
export function createTraceState(rawTraceState?: string): TraceState {

‎packages/opentelemetry-core/src/trace/TraceState.ts‎

Lines changed: 118 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* SPDX-License-Identifier: Apache-2.0
44
*/
55

6-
import type * as api from '@opentelemetry/api';
6+
import type { TraceState as TraceStateApi } from '@opentelemetry/api';
77
import { validateKey, validateValue } from '../internal/validators';
88

99
const MAX_TRACE_STATE_ITEMS = 32;
@@ -20,80 +20,144 @@ const LIST_MEMBER_KEY_VALUE_SPLITTER = '=';
2020
* - The value of any key can be updated. Modified keys MUST be moved to the
2121
* beginning of the list.
2222
*/
23-
export class TraceState implements api.TraceState {
24-
private _internalState: Map<string, string> = new Map();
23+
export class TraceState implements TraceStateApi {
24+
private _length: number;
25+
private _rawTraceState: string;
26+
private _internalState: Map<string, string> | undefined;
2527

2628
constructor(rawTraceState?: string) {
27-
if (rawTraceState) this._parse(rawTraceState);
29+
this._rawTraceState =
30+
typeof rawTraceState === 'string' ? rawTraceState : '';
31+
this._length = this._rawTraceState.length;
2832
}
2933

3034
set(key: string, value: string): TraceState {
31-
// TODO: Benchmark the different approaches(map vs list) and
32-
// use the faster one.
33-
const traceState = this._clone();
34-
if (traceState._internalState.has(key)) {
35-
traceState._internalState.delete(key);
35+
if (!validateKey(key) || !validateValue(value)) {
36+
return this;
3637
}
37-
traceState._internalState.set(key, value);
38-
return traceState;
38+
39+
const currState = this._getState();
40+
const currValue = currState.get(key);
41+
// Get the new length depending if we already have a value or not
42+
// - for existing keys we add the difference between the length of the values
43+
// - for new keys is the key & value lenght plus
44+
// - +1 for the key/value splitter
45+
// - +1 for the separator if there are other keys
46+
let newLength = this._length;
47+
if (typeof currValue === 'string') {
48+
newLength += value.length - currValue.length;
49+
} else {
50+
newLength += key.length + value.length + (currState.size > 0 ? 2 : 1);
51+
}
52+
if (newLength > MAX_TRACE_STATE_LEN) {
53+
return this;
54+
}
55+
56+
const newState = new Map(currState);
57+
newState.delete(key);
58+
newState.set(key, value);
59+
return this._fromState(newState, newLength);
3960
}
4061

4162
unset(key: string): TraceState {
42-
const traceState = this._clone();
43-
traceState._internalState.delete(key);
44-
return traceState;
63+
const currState = this._getState();
64+
const currValue = currState.get(key);
65+
66+
// No need to create a new instance if the key does not exist
67+
if (typeof currValue !== 'string') {
68+
return this;
69+
}
70+
71+
// Get the new length depending if we already have a value or not
72+
// - for existing keys we substract key and value length plus
73+
// - +1 for the key/value splitter
74+
// - +1 for the separator if there are other keys
75+
let newLength = this._length - (key.length + currValue.length + 1);
76+
if (currState.size > 1) {
77+
// remove separator from length if there's no key or only one.
78+
newLength = newLength - 1;
79+
}
80+
81+
const newState = new Map(currState);
82+
newState.delete(key);
83+
return this._fromState(newState, newLength);
4584
}
4685

4786
get(key: string): string | undefined {
48-
return this._internalState.get(key);
87+
const currState = this._getState();
88+
return currState.get(key);
4989
}
5090

5191
serialize(): string {
52-
return this._keys()
53-
.reduce((agg: string[], key) => {
54-
agg.push(key + LIST_MEMBER_KEY_VALUE_SPLITTER + this.get(key));
55-
return agg;
56-
}, [])
57-
.join(LIST_MEMBERS_SEPARATOR);
92+
// Maps put new entries at the end. We prepend the seralized entry
93+
// to get the right order according to the spec (updated members go 1st)
94+
let serialized = '';
95+
let index = 0;
96+
for (const entry of this._getState()) {
97+
if (index > 0) {
98+
serialized = LIST_MEMBERS_SEPARATOR + serialized;
99+
}
100+
serialized =
101+
`${entry[0]}${LIST_MEMBER_KEY_VALUE_SPLITTER}${entry[1]}` + serialized;
102+
index++;
103+
}
104+
return serialized;
58105
}
59106

60-
private _parse(rawTraceState: string) {
61-
if (rawTraceState.length > MAX_TRACE_STATE_LEN) return;
62-
this._internalState = rawTraceState
63-
.split(LIST_MEMBERS_SEPARATOR)
64-
.reverse() // Store in reverse so new keys (.set(...)) will be placed at the beginning
65-
.reduce((agg: Map<string, string>, part: string) => {
66-
const listMember = part.trim(); // Optional Whitespace (OWS) handling
67-
const i = listMember.indexOf(LIST_MEMBER_KEY_VALUE_SPLITTER);
68-
if (i !== -1) {
69-
const key = listMember.slice(0, i);
70-
const value = listMember.slice(i + 1, part.length);
71-
if (validateKey(key) && validateValue(value)) {
72-
agg.set(key, value);
73-
} else {
74-
// TODO: Consider to add warning log
75-
}
76-
}
77-
return agg;
78-
}, new Map());
79-
80-
// Because of the reverse() requirement, trunc must be done after map is created
81-
if (this._internalState.size > MAX_TRACE_STATE_ITEMS) {
82-
this._internalState = new Map(
83-
Array.from(this._internalState.entries())
84-
.reverse() // Use reverse same as original tracestate parse chain
85-
.slice(0, MAX_TRACE_STATE_ITEMS)
86-
);
107+
private _getState(): Map<string, string> {
108+
if (this._internalState) {
109+
return this._internalState;
110+
}
111+
112+
// Not parsed yet, lets do it
113+
const vendorMembers = this._rawTraceState.split(LIST_MEMBERS_SEPARATOR);
114+
115+
// This Map will have the order reversed
116+
const vendorEntries = new Map();
117+
let currentLength = 0;
118+
119+
for (const member of vendorMembers) {
120+
const m = member.trim();
121+
const idx = m.indexOf(LIST_MEMBER_KEY_VALUE_SPLITTER);
122+
if (idx === -1) {
123+
continue;
124+
}
125+
126+
const key = m.slice(0, idx);
127+
const value = m.slice(idx + 1);
128+
if (!validateKey(key) || !validateValue(value)) {
129+
continue;
130+
}
131+
132+
// Skip if adding the new member exceeds the length
133+
const futureLength =
134+
currentLength + m.length + (vendorEntries.size > 0 ? 1 : 0);
135+
if (futureLength > MAX_TRACE_STATE_LEN) {
136+
continue;
137+
}
138+
139+
// All good, add it
140+
vendorEntries.set(key, value);
141+
currentLength = futureLength;
142+
143+
// Check if we reached the max items
144+
if (vendorEntries.size >= MAX_TRACE_STATE_ITEMS) {
145+
break;
146+
}
87147
}
88-
}
89148

90-
private _keys(): string[] {
91-
return Array.from(this._internalState.keys()).reverse();
149+
// Now we set the length & the Map in the right order
150+
this._length = currentLength;
151+
this._internalState = new Map(
152+
Array.from(vendorEntries.entries()).reverse()
153+
);
154+
return this._internalState;
92155
}
93156

94-
private _clone(): TraceState {
95-
const traceState = new TraceState();
96-
traceState._internalState = new Map(this._internalState);
157+
private _fromState(state: Map<string, string>, length: number): TraceState {
158+
const traceState = Object.create(TraceState.prototype) as TraceState;
159+
traceState._internalState = state;
160+
traceState._length = length;
97161
return traceState;
98162
}
99163
}

‎packages/opentelemetry-core/test/common/trace/tracestate.test.ts‎

Lines changed: 93 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,70 @@ describe('TraceState', () => {
3939
assert.deepStrictEqual(state.serialize(), 'a=1');
4040
assert.strictEqual(orgState.serialize(), 'c=4,b=3,a=1');
4141
});
42+
43+
it('must not create a new TraceState when the entry does not exist', () => {
44+
const orgState = new TraceState('c=4,b=3,a=1');
45+
const state = orgState.unset('d');
46+
assert.strictEqual(orgState, state);
47+
assert.deepStrictEqual(orgState.serialize(), 'c=4,b=3,a=1');
48+
});
49+
50+
describe('when adding a new list member', () => {
51+
it('must not create a new TraceState if the value is to long', () => {
52+
const orgState = new TraceState('a=1,b=2');
53+
const state = orgState.set('c', '3'.repeat(257));
54+
assert.strictEqual(orgState, state);
55+
assert.deepStrictEqual(orgState.serialize(), 'a=1,b=2');
56+
});
57+
58+
it('must not create a new TraceState if makes exceed the total limit', () => {
59+
const tracestate = 'a=' + '1'.repeat(200) + ',b=' + '2'.repeat(200);
60+
const orgState = new TraceState(tracestate);
61+
const state = orgState.set('c', '3'.repeat(200)); // this makes it exceed 512
62+
assert.strictEqual(orgState, state);
63+
assert.deepStrictEqual(orgState.serialize(), tracestate);
64+
});
65+
});
66+
67+
describe('when updating a list member', () => {
68+
it('must not create a new TraceState if the value is to long', () => {
69+
const orgState = new TraceState('a=1,b=2');
70+
const state = orgState.set('b', '3'.repeat(257));
71+
assert.strictEqual(orgState, state);
72+
assert.deepStrictEqual(orgState.serialize(), 'a=1,b=2');
73+
});
74+
75+
it('must not create a new TraceState if makes exceed the total limit', () => {
76+
const tracestate = 'a=' + '1'.repeat(200) + ',b=' + '2'.repeat(200);
77+
const orgState = new TraceState(tracestate);
78+
const state = orgState.set('c', '3'.repeat(200)); // this makes it exceed 512
79+
assert.strictEqual(orgState, state);
80+
assert.deepStrictEqual(orgState.serialize(), tracestate);
81+
});
82+
});
4283
});
4384

4485
describe('.parse()', () => {
86+
it('must remove invalid entries when serializing', () => {
87+
// valid
88+
let tracestate = 'a=1';
89+
let state = new TraceState(tracestate);
90+
assert.deepStrictEqual(state.serialize(), tracestate);
91+
92+
// invalid: value exceeds 256 chars
93+
tracestate = 'a=' + 'b'.repeat(512);
94+
state = new TraceState(tracestate);
95+
assert.deepStrictEqual(state.serialize(), '');
96+
97+
// invalid: too many entries
98+
tracestate = new Array(32)
99+
.fill(0)
100+
.map((_: null, num: number) => `a${num}=${num}`)
101+
.join(',');
102+
state = new TraceState(tracestate + ',a32=32');
103+
assert.deepStrictEqual(state.serialize(), tracestate);
104+
});
105+
45106
it('must successfully parse valid state value', () => {
46107
const state = new TraceState(
47108
'vendorname2=opaqueValue2,vendorname1=opaqueValue1'
@@ -60,17 +121,25 @@ describe('TraceState', () => {
60121
assert.deepStrictEqual(state.serialize(), '');
61122
});
62123

63-
it('must drop states which cannot be parsed', () => {
124+
it('must skip list-members when the value is too long', () => {
125+
const state = new TraceState('a=' + 'b'.repeat(512) + ',c=d');
126+
assert.deepStrictEqual(state.get('a'), undefined);
127+
assert.deepStrictEqual(state.get('c'), 'd');
128+
assert.deepStrictEqual(state.serialize(), 'c=d');
129+
});
130+
131+
it('must skip list-members that cannot be parsed', () => {
64132
const state = new TraceState('a=1,b,c=3');
65133
assert.deepStrictEqual(state.get('a'), '1');
66134
assert.deepStrictEqual(state.get('b'), undefined);
67135
assert.deepStrictEqual(state.get('c'), '3');
68136
assert.deepStrictEqual(state.serialize(), 'a=1,c=3');
69137
});
70138

71-
it('must skip states that only have a single value with an equal sign', () => {
72-
const state = new TraceState('a=1=');
139+
it('must skip list-members that only have a single value with an equal sign', () => {
140+
const state = new TraceState('a=1=,b=2');
73141
assert.deepStrictEqual(state.get('a'), undefined);
142+
assert.deepStrictEqual(state.get('b'), '2');
74143
});
75144

76145
it('must successfully parse valid state keys', () => {
@@ -88,20 +157,23 @@ describe('TraceState', () => {
88157
});
89158

90159
it('must truncate states with too many items', () => {
91-
const state = new TraceState(
92-
new Array(33)
93-
.fill(0)
94-
.map((_: null, num: number) => `a${num}=${num}`)
95-
.join(',')
96-
);
97-
assert.deepStrictEqual(state['_keys']().length, 32);
98-
assert.deepStrictEqual(state.get('a0'), '0');
99-
assert.deepStrictEqual(state.get('a31'), '31');
100-
assert.deepStrictEqual(
101-
state.get('a32'),
102-
undefined,
103-
'should truncate from the tail'
104-
);
160+
const tracestate = new Array(33)
161+
.fill(0)
162+
.map((_: null, num: number) => `a${num}=${num}`);
163+
const state = new TraceState(tracestate.join(','));
164+
165+
tracestate.forEach((member, index) => {
166+
const [key, value] = member.split('=');
167+
if (index < 32) {
168+
assert.deepStrictEqual(state.get(key), value);
169+
} else {
170+
assert.deepStrictEqual(
171+
state.get(key),
172+
undefined,
173+
'should truncate from the tail'
174+
);
175+
}
176+
});
105177
});
106178

107179
it('should not count invalid items towards max limit', () => {
@@ -116,12 +188,13 @@ describe('TraceState', () => {
116188

117189
const state = new TraceState(tracestate.join(','));
118190

119-
assert.deepStrictEqual(state['_keys']().length, 32);
120-
assert.deepStrictEqual(state.get('a0'), '0');
121-
assert.deepStrictEqual(state.get('a31'), '31');
191+
assert.deepStrictEqual(state.get('invalid.prefix.key'), undefined);
122192
assert.deepStrictEqual(state.get('invalid.middle.key.a'), undefined);
123193
assert.deepStrictEqual(state.get('invalid.middle.key.b'), undefined);
124194
assert.deepStrictEqual(state.get('invalid.middle.key.c'), undefined);
195+
for (let i = 0; i < 32; i++) {
196+
assert.deepStrictEqual(state.get(`a${i}`), `${i}`);
197+
}
125198
});
126199
});
127200
});

0 commit comments

Comments
 (0)
⚡