Validate new Members only once, when they are added to the Baggage

[dmathieu]

This is an attempt at improving the number of calls to validate() in baggage.Member, calling it only when the member is added to the baggage.
Right now, we call it twice. Once within baggage.NewMember, and another time within baggage.New.

Closes #2498

I chose to remove the validate() within NewMember, as we create members in a couple other places, which could not too hardly end up with members which are never validated.

[codecov]

Codecov Report

Merging #2505 (b427b34) into main (f24f52a) will decrease coverage by 0.0%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            main   #2505     +/-   ##
=======================================
- Coverage   76.0%   75.9%   -0.1%     
=======================================
  Files        174     174             
  Lines      12084   12081      -3     
=======================================
- Hits        9189    9180      -9     
- Misses      2651    2656      +5     
- Partials     244     245      +1     

Impacted Files	Coverage Δ
baggage/baggage.go	98.2% <ø> (-0.1%)	⬇️
sdk/metric/sdk.go	80.0% <0.0%> (-1.5%)	⬇️
sdk/trace/batch_span_processor.go	79.4% <0.0%> (-1.0%)	⬇️

[dmathieu]

Note about the changelog failure: this isn't changing any outside behavior.
So I suppose the Skip changelog tag could be added.

[MrAlias]

I'm hesitant to accept this approach. It will indeed accommodate the related issues use case, but I worry users that want to be informed early about invalid data will not appreciate this.

Instead, could we utilize a sync.Once function as an added field of a Member to validate the member only once and return a saved response if called multiple times?

If we decide to go this route, we can also do the same for the properties type.

[dmathieu]

That makes sense. But once.Do doesn't allow returning data, so it requires storing the value it generated somewhere.
We can't store it within the member/property struct though, as those aren't pointers.

[jmacd]

TraceState has a similar "excessive validation" problem, shown in open-telemetry/opentelemetry-go-contrib#1379 benchmarks.

In general, I support implementations that "trust the user" not to make mistakes in cases where excessive validation is a problem. @dmathieu would you be happy with an alternate constructor that "trusts the user" for baggage to avoid double validation? I favor this approach for the problem w/ TraceState double validation mentioned above.

[dmathieu]

You mean keep doing the validation in NewMember and remove it from New (which creates the Baggage)?
As mentioned before, I didn't go with that approach, because it felt like it would be relatively easy to get an unvalidated member. But I don't mind going that route anyway.

[MadVikingGod]

We are somewhat limited in how far we can trust users because we are attempting to stay compliant with both the OTEL and the W3C spec for baggage. Mainly we can't output invalid data, even if the receivers are supposed to accept it.

So I think we are in this bind because we don't want to handle a special case in most of our baggage logic.

There are two ways to make a Member: NewMember() and Memeber{}/new(Member). The former we do validation, but there is no way for us to validate the Empty Member, which happens to be an invalid W3C Baggage list-member (it is missing 1 character for the key).

So if we push the validation to only at creation (NewMember()) we do get the benefits you describe, but then we also have to handle skipping any Member{}'s we find when we create baggage, and when we transport it forward.

An alternative position is that instead of doing a sync.Once we just have a isValid bool member. This only being set in NewMember would have the same effect, and would let us skip at the baggage creation.

[MrAlias]

An alternative position is that instead of doing a sync.Once we just have a isValid bool member. This only being set in NewMember would have the same effect, and would let us skip at the baggage creation.

Agreed. Set isValid when it is created if it is valid and only validate later if not true.

[dmathieu]

I suppose this approach can be closed in favor of #2522.