Replace AutoValue usage in baggage with final concrete classes.

[anuraaga]

Related to #1925. I think making public classes in the API final is a good idea, but we currently have many public abstract classes for the use of AutoValue. I think we should be consistent for public API in that all extensible classes are interface, and all non-extensible ones are final. If that means implementing (e.g., hitting some keys in IntelliJ) these methods, I think it's better than the current inconsistency.

Here I went with making baggage classes concrete as an example - alternatively splitting interfaces and package-private AutoValue classes would make the extensibility consistent with other interfaces.

[anuraaga]

Changed this a bit, I think the previous autovalue implementation would have been EntryMetadata{metadata=foo}

[jkwatson]

I think the old version was a bit better. Any time we make a toString() look too usable by an end-user for something other than debugging, I think we open ourselves up to people mis-using it, which would lock us into a particular implementation.

[codecov]

Codecov Report

Merging #1937 into master will increase coverage by 0.08%.
The diff coverage is 97.61%.

@@             Coverage Diff              @@
##             master    #1937      +/-   ##
============================================
+ Coverage     86.63%   86.71%   +0.08%     
- Complexity     1501     1521      +20     
============================================
  Files           184      184              
  Lines          5662     5698      +36     
  Branches        582      587       +5     
============================================
+ Hits           4905     4941      +36     
  Misses          551      551              
  Partials        206      206              

Impacted Files	Coverage Δ	Complexity Δ
.../api/baggage/propagation/W3CBaggagePropagator.java	93.18% <ø> (ø)	15.00 <0.00> (ø)
...ava/io/opentelemetry/api/trace/PropagatedSpan.java	79.16% <ø> (ø)	17.00 <0.00> (ø)
.../java/io/opentelemetry/context/DefaultContext.java	100.00% <ø> (ø)	7.00 <0.00> (ø)
...ain/java/io/opentelemetry/context/LazyStorage.java	88.46% <ø> (ø)	9.00 <0.00> (ø)
.../main/java/io/opentelemetry/api/baggage/Entry.java	96.29% <95.45%> (+10.58%)	17.00 <12.00> (+11.00)
...va/io/opentelemetry/api/baggage/EntryMetadata.java	100.00% <100.00%> (ø)	11.00 <11.00> (+8.00)
...io/opentelemetry/api/baggage/ImmutableBaggage.java	96.77% <100.00%> (ø)	17.00 <0.00> (ø)
...opentelemetry/opentracingshim/SpanContextShim.java	88.57% <100.00%> (ø)	9.00 <0.00> (ø)
...va/io/opentelemetry/opentracingshim/TraceShim.java	100.00% <100.00%> (ø)	4.00 <1.00> (+1.00)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6f111a...e1c4cd2. Read the comment docs.

[anuraaga]

@trask Maybe can leave a note too, I guess any final class we introduce will make the bridging in the agent harder so maybe just Interface for all! is a better approach for our instrumentation.

[bogdandrutu]

Ok, I think you misinterpreted the section about final in the guideline. The idea is that we should not let user extend the class because we don't want to deal with unexpected behavior, but in this class this cannot be extended because the ctor is package protected.

Also I would not make the API decisions based on auto-instrumentation.

[bogdandrutu]

So in this case, by making Baggage an interface forces us to deal with cases where a third-party implementation of the interface is passed to our APIs, and that may complicate things unnecessary and may disallow future improvements that we can make.

[jkwatson]

So in this case, by making Baggage an interface forces us to deal with cases where a third-party implementation of the interface is passed to our APIs, and that may complicate things unnecessary and may disallow future improvements that we can make.

I've heard this comment a lot, but I've never heard of an actual example of this happening in the real world. Can you show a case where you've been actually impacted by this in the past? Java 8 default methods let us evolve interfaces in a similar way that we've been able to evolve abstract classes, pre Java-8.

[Oberon00]

@bogdandrutu Have you seen #1935 "Split SpanContext into interface / impl"? Are you opposed to that too?

I don't think we should to be prepared for different implementations though, this is just a crutch for the auto-instrumenation.

[jkwatson]

Personally, I'd much rather go the interface -> private autoValue implementation [note: this could be a private inner class of the interface, maybe?] over having to maintain equals/hashcode/toString, especially since the consequences of mis-managing equals/hashcode can be catastrophic.

[jkwatson]

Personally, I'd much rather go the interface -> private autoValue implementation [note: this could be a private inner class of the interface, maybe?] over having to maintain equals/hashcode/toString, especially since the consequences of mis-managing equals/hashcode can be catastrophic.

Note: can't do it with a private inner class on an interface, because you can't have private inner classes on interfaces. 🤦

[anuraaga]

Filed #1946 to discuss interfaces vs classes.

[bogdandrutu]

I've heard this comment a lot, but I've never heard of an actual example of this happening in the real world. Can you show a case where you've been actually impacted by this in the past? Java 8 default methods let us evolve interfaces in a similar way that we've been able to evolve abstract classes, pre Java-8.

FYI: This is not about evolving the API, but about dealing with third-party implementation of the interface. I am advocating to not allow other implementations for classes that we don't need to offer that capability.

Yes, we had the problem in OpenCensus. We had an interface that represented the Tags (a.k.a baggage) that did not expose publicly a get method, and have our own implementation that exposed (package protected) a get method. Now because that was an interface and everyone can extend the interface it was not trivial to convert to the internal implementation because we had a path where we have to deal with another implementation that does not have our functionality.

You may argue that all functionality must be public, but that is not necessary true if you don't want user to have access to that or abuse your API. By restricting who can implement an interface/abstract class you can rely on package protected functionality to optimize performance etc.

So I am fine with using final classes or auto-value classes with package protected ctor, because it means we control the implementation.

[jkwatson]

Yes, we had the problem in OpenCensus. We had an interface that represented the Tags (a.k.a baggage) that did not expose publicly a get method, and have our own implementation that exposed (package protected) a get method. Now because that was an interface and everyone can extend the interface it was not trivial to convert to the internal implementation because we had a path where we have to deal with another implementation that does not have our functionality.

You may argue that all functionality must be public, but that is not necessary true if you don't want user to have access to that or abuse your API. By restricting who can implement an interface/abstract class you can rely on package protected functionality to optimize performance etc.

It sounds like you were casting to your implementation below the API surface? Yes, that does sound like it would get you in trouble, for sure. I don't think that's a good reason not to use interfaces in your API, though. You just need to make sure you don't rely on special implementation-only functionality if you do expose your data as an interface.

In general, I think if what you are exposing is pure data, then having a final class is fine. If you're exposing functionality, I will always prefer to have an interface for that functionality.

In the particular case of Baggage... We have pure, immutable data, so I'm totally 100% good with having a final class at the API layer for people to use.

Note: I'll also update the linked issue around interfaces with these comments. :)

[anuraaga]

I don't think this PR is necessary since indeed the private constructor, which I forgot about, does make it effectively final. But still continuing the discussion on interfaces!