refactor!: specification compliant baggage

[dyladan]

This is a pretty big change to baggage making it spec compliant, but fortunately baggage isn't used in too many places.

[codecov]

Codecov Report

Merging #1876 (cdbae14) into main (7e423a9) will decrease coverage by 0.28%.
The diff coverage is 98.27%.

@@            Coverage Diff             @@
##             main    #1876      +/-   ##
==========================================
- Coverage   92.72%   92.44%   -0.29%     
==========================================
  Files         174      159      -15     
  Lines        6038     5146     -892     
  Branches     1283     1089     -194     
==========================================
- Hits         5599     4757     -842     
+ Misses        439      389      -50     

Impacted Files	Coverage Δ
.../opentelemetry-api/src/baggage/internal/baggage.ts	96.15% <96.15%> (ø)
packages/opentelemetry-api/src/baggage/index.ts	100.00% <100.00%> (ø)
...s/opentelemetry-api/src/baggage/internal/symbol.ts	100.00% <100.00%> (ø)
...emetry-core/src/baggage/propagation/HttpBaggage.ts	98.21% <100.00%> (+0.10%)	⬆️
...ackages/opentelemetry-shim-opentracing/src/shim.ts	87.70% <100.00%> (+0.10%)	⬆️
...kages/opentelemetry-exporter-collector/src/util.ts
...mentation-xml-http-request/src/enums/EventNames.ts
...ry-exporter-collector/src/CollectorExporterBase.ts
packages/opentelemetry-web/src/utils.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
... and 16 more

[dyladan]

@open-telemetry/javascript-approvers this one is important because it will be a part of the API 1.0 and is a pretty drastic change from what we had. Please look over it.

[Flarna]

Some tests would be nice. And please update the breaking changes section in readme.

[dyladan]

Some tests would be nice. And please update the breaking changes section in readme.

I was just working on tests :)

[obecny]

First pass - commented on few issues.
I think we should have unit tests for cases when baggage is separated with spaces
https://www.w3.org/TR/baggage/#examples-of-http-headers

[obecny]

ImhogetAllEntries should return map of getEntry. Because getEntry returns BaggageEntry it should return Map<key, BaggageEntry> not [string, BaggageEntry][]. Or name the function for example getEntriesAsArray etc. WDYT ?

[dyladan]

I just thought the most common reason to use getAllEntries would be to iterate over all of them for serialization. I can change it to map if you prefer. This is especially easy since order doesn't matter in baggage.

[mwear]

Generally this looks good and appears to be on the right track. One thing that bothered me when I was working on the Ruby version of this, is that you need to create a new baggage instance for each modification of baggage. I wanted a way to be able to make multiple changes and then create a new baggage instance with those changes applied. To do this, I introduced a builder.

For example, you have to create three baggage instances in order to remove one key and add two new keys:

const updated = baggage.removeEntry('foo)
                       .setEntry('bar', { value: 'baz' })
                       .setEntry('baz', { value: 'quux' });

With a builder, you can batch changes and return a baggage instance with the changes applied. For example:

baggage.build(b => {
    b.removeEntry('foo);
    b.setEntry('bar', { value: 'baz' });
    b.setEntry('baz', { value: 'quux' });
});

I'm not sure if it's essential to have something like this, and it can always be added later, but I thought it'd be worth mentioning.

[obecny]

One thing which confuses me most is why this._entries are never changed during all operations. Shouldn't the entries be treated as the baggage state from which you can remove or add ?. As this way if I have this

const baggage = new BaggageImpl();
baggage.setEntry('a',{b:1});
console.log(baggage.getEntry('a')); // undefined

Wheres this baggage should return the entry that has been just set.
I checked java for example and they are removing or adding this too.
https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/main/java/io/opentelemetry/api/baggage/ImmutableBaggage.java#L98

Python is doing similar thing
https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/baggage/__init__.py#L76
But instead of keeping the state of baggage inside class they are updating the baggage directly in context - so context is required. But still you have one place where the baggage state is kept and entries are being removed or added.

We can keep the baggage state inside baggage or in context but I think functions should modify the state anyway so that you can reference the baggage and have updated state of it whenever it changes.

[obecny]

After thinking for a while I'm hesitant to say that keeping state of baggage in context as python did makes a lot of sense, WDYT ?

[dyladan]

Will address @obecny's comments first

One thing which confuses me most is why this._entries are never changed during all operations. Shouldn't the entries be treated as the baggage state from which you can remove or add ?. As this way if I have this

const baggage = new BaggageImpl();
baggage.setEntry('a',{b:1});
console.log(baggage.getEntry('a')); // undefined

Spec states baggage is immutable:

The Baggage container MUST be immutable, so that the containing Context also remains immutable.

link to spec

Wheres this baggage should return the entry that has been just set.
I checked java for example and they are removing or adding this too.
open-telemetry/opentelemetry-java@main/api/all/src/main/java/io/opentelemetry/api/baggage/ImmutableBaggage.java#L98

Python is doing similar thing
open-telemetry/opentelemetry-python@main/opentelemetry-api/src/opentelemetry/baggage/init.py#L76
But instead of keeping the state of baggage inside class they are updating the baggage directly in context - so context is required. But still you have one place where the baggage state is kept and entries are being removed or added.

Both of these are not doing what you think. In python, they create a new dict object and modify only the new one. They also create a new context. In java, you linked the builder. After a context has been built it is immutable.

We can keep the baggage state inside baggage or in context but I think functions should modify the state anyway so that you can reference the baggage and have updated state of it whenever it changes.

Functions cannot modify the state. See the link in the first point

[dyladan]

Generally this looks good and appears to be on the right track. One thing that bothered me when I was working on the Ruby version of this, is that you need to create a new baggage instance for each modification of baggage. I wanted a way to be able to make multiple changes and then create a new baggage instance with those changes applied. To do this, I introduced a builder.

For example, you have to create three baggage instances in order to remove one key and add two new keys:

const updated = baggage.removeEntry('foo)
                       .setEntry('bar', { value: 'baz' })
                       .setEntry('baz', { value: 'quux' });

With a builder, you can batch changes and return a baggage instance with the changes applied. For example:

baggage.build(b => {
    b.removeEntry('foo);
    b.setEntry('bar', { value: 'baz' });
    b.setEntry('baz', { value: 'quux' });
});

I'm not sure if it's essential to have something like this, and it can always be added later, but I thought it'd be worth mentioning.

I added a clear() and removeEntries(...keys) method.

I think creating a builder might be something we can consider later, but for now we need to get a spec compliant baggage out the door so it doesn't block GA.

[obecny]

lgtm

[mwear]

LGTM