Opentelemetry-java: Extending HashMap may be a problem for AttributesMap

[bogdandrutu]

Currently AttributesMap extends HashMap which may cause troubles.

We need to re-evaluate if this is a real problem or we can live with this anti-pattern. Wrapping the hash-map adds an extra unnecessary allocation.

[trask]

Just my 2c, I'm not opposed to extending HashMap if it's not for a public class, and would personally prefer that over the extra allocation of wrapping the HashMap.

An example I ran into recently while looking at Reactor Context does the same:
https://github.com/reactor/reactor-core/blob/master/reactor-core/src/main/java/reactor/util/context/ContextN.java#L31

[carlosalberto]

I'm not opposed to extending HashMap if it's not for a public class

Same feeling here. I'd be more worried if this indeed was a public class.

[jkwatson]

If we want to keep this, let's be very sure we don't let it proliferate into public APIs.

[trask]

maybe a comment? 😄

// EXTENDING HASHMAP IS VERY BAD FOR A PUBLIC API
// THIS CLASS IS NOT PUBLIC SO IT IS ACCEPTABLE HERE AS AN OPTIMIZATION
// DO NOT MAKE THIS CLASS PUBLIC WITHOUT CHANGING ITS SUPER TYPE
final class AttributesMap extends HashMap<String, AttributeValue> {

[jkwatson]

Only if we put "ACCEPTABLE" in quotes.

[jkwatson]

this has been fixed and the AttributesMap now wraps the map, rather than exposing it. closing!