Prototype: Complex attributes (Option C)#7813
Prototype: Complex attributes (Option C)#7813trask wants to merge 6 commits intoopen-telemetry:mainfrom
Conversation
e39d49b to
a8085a5
Compare
| * | ||
| * @return this Builder | ||
| */ | ||
| default AttributesBuilder put(String key, Value<?> value) { |
There was a problem hiding this comment.
hm, should these be identical?
put("abc", Value.of("xyz"))put("abc", "xyz")
they should be identical over OTLP
but should they be exposed identically to in-memory processors?
specifically, is the following behavior ok for put("abc", Value.of("xyz"))
Attributes.get(stringKey("abc"))returnsnullAttributes.forEach()andAttributes.asMap()both represent the key value pair as an attribute key with AttributeTypeVALUEand a value with typeValue
I think this is ok, since you're explicitly opting in to an AttributeKey with AttributeType VALUE when you're setting the attribute
Oh wait, maybe this is a good reason not to have this overload put(String, Value<?>) and require calling put(AttributeKey, Value<?>).
There was a problem hiding this comment.
but should they be exposed identically to in-memory processors?
I think yes since if they are not identical, then every in-memory processors has extra work to do if they want to perform the simple task of accessing a scalar value.
Oh wait, maybe this is a good reason not to have this overload put(String, Value) and require calling put(AttributeKey, Value)
This isn't a good enough defense because a caller could just call put(AttriubteKey.valueKey("abc"), Value.of("xyz") and achieve the same affect.
We could have the default implementation of put(String, Value) and put(AttributeKey, Value):
- Check if the value is a primitive or array of primitives
- If so, call the equivalent API (e.g.
put(String, String)for string) - Else the value is complex (map, array of maps, etc) and can be left as a
Value
There was a problem hiding this comment.
It's my hot take that any time one finds oneself re/inventing a type system in a language to try and work around the language's type system shortcomings, you're in for a bad time. 😬
But yeah, if we go this route, the a value of type String must be necessarily different from a Value of type String. So yeah, ValueString (which isA Value<String>) does not have semantic equivalence to a String string.
I'm sure this will trip up someone, or at least cause them to cuss about this, but it doesn't seem like a deal breaker to me.
...or as was already mentioned, requires other code to do more work to check.
There was a problem hiding this comment.
- Check if the value is a primitive or array of primitives
- If so, call the equivalent API (e.g.
put(String, String)for string)
I drafted javadoc to help us evaluate this option: fa87c47
This comment was marked as resolved.
This comment was marked as resolved.
0fd4988 to
fa87c47
Compare
|
Closing as incubating implementation PR is ready: #7814 |
Related
Pros:
Cons:
Value-based maps ergonomics are not as good asAttributesValue-based maps are less performant compared toAttributes