From 5e412ac2d844614a920e7a5f8aa4f515e629dd9a Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Thu, 15 Oct 2020 11:13:15 +0200 Subject: [PATCH 1/2] fix: make TraceState immutable Spec requires that TraceState is immutable. Adapt TraceState API to return a TraceState for set/unset. Adapt implementation of set/unset to clone the TraceState before mutating and return the cloned TraceState. --- .../src/trace/trace_state.ts | 16 ++++++++------ .../src/trace/TraceState.ts | 22 ++++++++++++++----- .../test/trace/tracestate.test.ts | 18 +++++++-------- 3 files changed, 35 insertions(+), 21 deletions(-) diff --git a/packages/opentelemetry-api/src/trace/trace_state.ts b/packages/opentelemetry-api/src/trace/trace_state.ts index 074633633e..640d1578a9 100644 --- a/packages/opentelemetry-api/src/trace/trace_state.ts +++ b/packages/opentelemetry-api/src/trace/trace_state.ts @@ -13,23 +13,25 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + export interface TraceState { /** - * Adds or updates the TraceState that has the given `key` if it is - * present. The new State will always be added in the front of the - * list of states. + * Create a new TraceState which inherits from this TraceState and has the + * given key set. + * The new entry will always be added in the front of the list of states. * * @param key key of the TraceState entry. * @param value value of the TraceState entry. */ - set(key: string, value: string): void; + set(key: string, value: string): TraceState; /** - * Removes the TraceState Entry that has the given `key` if it is present. + * Return a new TraceState which inherits from this TraceState but does not + * contain the given key. * - * @param key the key for the TraceState Entry to be removed. + * @param key the key for the TraceState entry to be removed. */ - unset(key: string): void; + unset(key: string): TraceState; /** * Returns the value to which the specified key is mapped, or `undefined` if diff --git a/packages/opentelemetry-core/src/trace/TraceState.ts b/packages/opentelemetry-core/src/trace/TraceState.ts index c7c18802e7..0d881b15c3 100644 --- a/packages/opentelemetry-core/src/trace/TraceState.ts +++ b/packages/opentelemetry-core/src/trace/TraceState.ts @@ -38,15 +38,21 @@ export class TraceState implements api.TraceState { if (rawTraceState) this._parse(rawTraceState); } - set(key: string, value: string): void { + set(key: string, value: string): TraceState { // TODO: Benchmark the different approaches(map vs list) and // use the faster one. - if (this._internalState.has(key)) this._internalState.delete(key); - this._internalState.set(key, value); + const traceState = this._clone(); + if (traceState._internalState.has(key)) { + traceState._internalState.delete(key); + } + traceState._internalState.set(key, value); + return traceState; } - unset(key: string): void { - this._internalState.delete(key); + unset(key: string): TraceState { + const traceState = this._clone(); + traceState._internalState.delete(key); + return traceState; } get(key: string): string | undefined { @@ -95,4 +101,10 @@ export class TraceState implements api.TraceState { private _keys(): string[] { return Array.from(this._internalState.keys()).reverse(); } + + private _clone(): TraceState { + const traceState = new TraceState(); + traceState._internalState = new Map(this._internalState); + return traceState; + } } diff --git a/packages/opentelemetry-core/test/trace/tracestate.test.ts b/packages/opentelemetry-core/test/trace/tracestate.test.ts index 2fc2a2a13a..67fe417a1c 100644 --- a/packages/opentelemetry-core/test/trace/tracestate.test.ts +++ b/packages/opentelemetry-core/test/trace/tracestate.test.ts @@ -25,17 +25,17 @@ describe('TraceState', () => { }); it('must replace keys and move them to the front', () => { - const state = new TraceState('a=1,b=2'); - state.set('b', '3'); + const orgState = new TraceState('a=1,b=2'); + const state = orgState.set('b', '3'); + assert.deepStrictEqual(orgState.serialize(), 'a=1,b=2'); assert.deepStrictEqual(state.serialize(), 'b=3,a=1'); }); it('must add new keys to the front', () => { - const state = new TraceState(); - state.set('vendorname1', 'opaqueValue1'); + let state = new TraceState().set('vendorname1', 'opaqueValue1'); assert.deepStrictEqual(state.serialize(), 'vendorname1=opaqueValue1'); - state.set('vendorname2', 'opaqueValue2'); + state = state.set('vendorname2', 'opaqueValue2'); assert.deepStrictEqual( state.serialize(), 'vendorname2=opaqueValue2,vendorname1=opaqueValue1' @@ -43,12 +43,12 @@ describe('TraceState', () => { }); it('must unset the entries', () => { - const state = new TraceState('c=4,b=3,a=1'); - state.unset('b'); + const orgState = new TraceState('c=4,b=3,a=1'); + let state = orgState.unset('b'); assert.deepStrictEqual(state.serialize(), 'c=4,a=1'); - state.unset('c'); - state.unset('A'); + state = state.unset('c').unset('A'); assert.deepStrictEqual(state.serialize(), 'a=1'); + assert.strictEqual(orgState.serialize(), 'c=4,b=3,a=1'); }); }); From a3caec43e6435c6922f1c70c4ccaf7737056d93f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerhard=20St=C3=B6bich?= Date: Wed, 21 Oct 2020 22:08:32 +0200 Subject: [PATCH 2/2] Update tracestate.test.ts --- packages/opentelemetry-core/test/trace/tracestate.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-core/test/trace/tracestate.test.ts b/packages/opentelemetry-core/test/trace/tracestate.test.ts index 67fe417a1c..8f4e850199 100644 --- a/packages/opentelemetry-core/test/trace/tracestate.test.ts +++ b/packages/opentelemetry-core/test/trace/tracestate.test.ts @@ -24,14 +24,14 @@ describe('TraceState', () => { assert.deepStrictEqual(state.serialize(), 'a=1,b=2'); }); - it('must replace keys and move them to the front', () => { + it('must create a new TraceState and move updated keys to the front', () => { const orgState = new TraceState('a=1,b=2'); const state = orgState.set('b', '3'); assert.deepStrictEqual(orgState.serialize(), 'a=1,b=2'); assert.deepStrictEqual(state.serialize(), 'b=3,a=1'); }); - it('must add new keys to the front', () => { + it('must create a new TraceState and add new keys to the front', () => { let state = new TraceState().set('vendorname1', 'opaqueValue1'); assert.deepStrictEqual(state.serialize(), 'vendorname1=opaqueValue1'); @@ -42,7 +42,7 @@ describe('TraceState', () => { ); }); - it('must unset the entries', () => { + it('must create a new TraceState and unset the entries', () => { const orgState = new TraceState('c=4,b=3,a=1'); let state = orgState.unset('b'); assert.deepStrictEqual(state.serialize(), 'c=4,a=1');