-
Notifications
You must be signed in to change notification settings - Fork 855
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.
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
Sketch out ExtendedAttributes #6983
base: main
Are you sure you want to change the base?
Conversation
|
||
@Override | ||
public Attributes getAttributes() { | ||
return getExtendedAttributes().asAttributes(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the copy isn't ideal from perf perspective, though maybe it could be optimized by returning a filtered view into the underlying extended attributes struct somehow
it seems like a good compromise from backcompat perspective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
though maybe it could be optimized by returning a filtered view into the underlying extended attributes struct somehow
Yeah I think there's some potential here. One challenge (as we saw with FilteredAttributes) is having an equals implementation that works against the standard Attributes implementation.
Ideally, all callers migrate to calling getExtendedAttributes
, which allows them to get around the performance hit. My intuition tells me there's not too many callers of this today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, all callers migrate to calling
getExtendedAttributes
, which allows them to get around the performance hit. My intuition tells me there's not too many callers of this today.
yeah, this makes sense to me. maybe just adding a javadoc warning on getAttributes (or even eventually deprecating it?) to encourage users to move away from it
this.attributes = | ||
AttributesMap.create( | ||
logLimits.getMaxNumberOfAttributes(), logLimits.getMaxAttributeValueLength()); | ||
return setAttribute(key.asExtendedAttributeKey(), value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could cache the ExtendedAttributeKey inside of the AttributeKey to avoid the perf hit here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could cache the ExtendedAttributeKey inside of the AttributeKey to avoid the perf hit here
Already doing this! See changes to InternalAttributesKeyImpl
<T> LogRecordBuilder setAttribute(AttributeKey<T> key, T value); | ||
|
||
/** TODO. */ | ||
default <T> LogRecordBuilder setAttribute(ExtendedAttributeKey<T> key, T value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect supporting setters for both kinds of attributes mitigates the primary concern here: open-telemetry/opentelemetry-specification#4201
it could also be a bit confusing when iterating over the extended attribute and testing if something is equal to one of our semconv constants, but that's only an issue when implementing SDK processors, so definitely not as important
Alternative to #6973, #6960, #6959, #6958.
In contrast to these other approaches, the key feature in this proposal is that we leave existing Attributes, AttributeKey, AttributeType alone. These represent the standard attributes and don't have any new situational surface area with the potential to confuse users.
Instead, this proposes we introduce new ExtendedAttribute, ExtendedAttributeKey, ExtendedAttributeType classes to represent the extended version of standard attribute used in log attributes.
The LogRecordBuilder API has overloads to accept either Attributes or ExtendedAttributes.
There are APIs for converting between Attribute / ExtendedAttribute where it makes sense:
semantic-convention-java
can be easily converted to the extended equivalentOn the SDK side, there's a new LogRecordData#getExtendedAttributes() method for exporters to access and encode extended attributes. For backwards compatibility, exporters can of course continue to access LogRecordData#getAttributes() which gets a filtered set of entries.
This is still a rough sketch, but it proves its possible to provide good API ergonomics for the extended attributes representation used in logs / events without leaking any details into the existing standard attributes representation.
My one problem with this is that ExtendedAttributes is more restricted that what the spec says about log record attributes, with supported types: STRING, BOOLEAN, LONG, DOUBLE, STRING_ARRAY, BOOLEAN_ARRAY, LONG_ARRAY, MAP, MAP_ARRAY. I don't feel comfortable introducing this new API without language from the spec that gives me confidence that this is sufficient (i.e. that we don't need to support something like heterogeneous arrays).
For API usage demosntration, see ExtendedAttributesTest
cc @trask