Prototype: Complex attributes (Option B)#7661
Prototype: Complex attributes (Option B)#7661trask wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
| DOUBLE_ARRAY, | ||
| BYTE_ARRAY, | ||
| VALUE_ARRAY, | ||
| VALUE_MAP |
There was a problem hiding this comment.
If I'm not mistaken, the only difference between this and Option A #7632 is the names of the enumeration?
Both of these options make use of the Value / KeyValue classes I introduced a while back, which is distinctly different than the #7123 where I adapted the Attributes / AttributesKey classes to support nested maps. Of course #7123 was incomplete and didn't show how to model other complex attribute types including byte array, object array, heterogeneous array.
Originally we weren't fond of the Value / KeyValue approach because of performance reasons... was there some conversation in open-telemetry/opentelemetry-specification#4485 that provided insight about the need for a AnyValue representation?
| return InternalAttributeKeyImpl.create(key, AttributeType.VALUE_ARRAY); | ||
| } | ||
|
|
||
| static AttributeKey<List<KeyValue>> valueMapKey(String key) { |
There was a problem hiding this comment.
Oh I see this is an additional difference. In #7632 this is represented as AttributeKey<Attributes>.
I think I lean slightly towards this approach: Its of course awkward to have both Value / KeyValue and Attributes which overlap in responsibilities. But we could evolve our thinking Attributes to be a convenient more convenient / type safe / performant way to represent the Value<List<Value<?>>> type. Then we could evolve Attributes, AttributesBuilder with convenience methods for better interop with Value / KeyValue. Things like:
- AttributesBuilder.put(KeyValue)
- AttributesBuilder.put(String, Value)
- Attributes.forEach(Consumer)
- Attributes.asKvList() -> List
- Attributes.asKvMap() -> Map<String, Value>
- etc
| return InternalAttributeKeyImpl.create(key, AttributeType.VALUE_ARRAY); | ||
| } | ||
|
|
||
| static AttributeKey<List<KeyValue>> valueMapKey(String key) { |
There was a problem hiding this comment.
The prototype doesn't show the value implementation readily, so I'll ask: Does using List<KeyValue> also suggest that the value of the attribute may contain more than one different KeyValues with the same key but different values? Does that make sense? I just mean that the list could (in theory) contain key duplicates.
- Does the spec address this, or maybe even allow it?
- If it's not expected/allowed, does it complicate the implementation to ensure unique keys in the value list?
|
Closing, I don't think this Option gives us anything noticeably better than the (new and) simpler #7813 |
3 participants
Related
Pros:
Cons:
List<KeyValue>is a little confusing