Skip to content

Prototype: Complex attributes (Option B) #7661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Prototype: Complex attributes (Option B) #7661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions api/all/src/main/java/io/opentelemetry/api/common/AttributeKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,16 @@ static AttributeKey<List<Long>> longArrayKey(String key) {
static AttributeKey<List<Double>> doubleArrayKey(String key) {
return InternalAttributeKeyImpl.create(key, AttributeType.DOUBLE_ARRAY);
}

static AttributeKey<byte[]> byteArrayKey(String key) {
return InternalAttributeKeyImpl.create(key, AttributeType.BYTE_ARRAY);
}

static AttributeKey<List<Value<?>>> valueArrayKey(String key) {
return InternalAttributeKeyImpl.create(key, AttributeType.VALUE_ARRAY);
}

static AttributeKey<List<KeyValue>> valueMapKey(String key) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return InternalAttributeKeyImpl.create(key, AttributeType.VALUE_MAP);
}
}
5 changes: 4 additions & 1 deletion api/all/src/main/java/io/opentelemetry/api/common/AttributeType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,8 @@ public enum AttributeType {
STRING_ARRAY,
BOOLEAN_ARRAY,
LONG_ARRAY,
DOUBLE_ARRAY
DOUBLE_ARRAY,
BYTE_ARRAY,
VALUE_ARRAY,
VALUE_MAP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
Loading