Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: make TraceState immutable #1597

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
16 changes: 9 additions & 7 deletions packages/opentelemetry-api/src/trace/trace_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 17 additions & 5 deletions packages/opentelemetry-core/src/trace/TraceState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
}
}
18 changes: 9 additions & 9 deletions packages/opentelemetry-core/test/trace/tracestate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,30 +25,30 @@ describe('TraceState', () => {
});

it('must replace keys and move them to the front', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should update the description and mention that, "creates a new tracestate and replace..." WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

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'
);
});

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');
});
});

Expand Down