Skip to content
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

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

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Dec 28, 2024

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:

  • AttributeKey#asExtendedAttributeKey() produces ExtendedAttributeKey, so attribute keys from semantic-convention-java can be easily converted to the extended equivalent
  • ExtendedAttributeKey#asAttributeKey() produces AttributeKey (or null if there is no equivalent AttributeKey)
  • ExtendedAttributes#asAttributes() produces Attributes which is filtered to entries which can be represented with standard attributes

On 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

Copy link

codecov bot commented Dec 28, 2024

Codecov Report

Attention: Patch coverage is 54.42478% with 103 lines in your changes missing coverage. Please review.

Project coverage is 89.30%. Comparing base (ee05c5c) to head (53d5439).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...elemetry/api/common/ExtendedAttributesBuilder.java 7.69% 35 Missing and 1 partial ⚠️
...api/internal/InternalExtendedAttributeKeyImpl.java 63.46% 12 Missing and 7 partials ⚠️
...i/common/ArrayBackedExtendedAttributesBuilder.java 34.78% 13 Missing and 2 partials ⚠️
.../io/opentelemetry/sdk/logs/ReadWriteLogRecord.java 0.00% 9 Missing ⚠️
...opentelemetry/api/common/ExtendedAttributeKey.java 36.36% 7 Missing ⚠️
...etry/api/common/ArrayBackedExtendedAttributes.java 87.50% 2 Missing and 1 partial ⚠️
...va/io/opentelemetry/api/logs/LogRecordBuilder.java 50.00% 2 Missing and 1 partial ⚠️
.../opentelemetry/sdk/logs/SdkReadWriteLogRecord.java 81.25% 2 Missing and 1 partial ⚠️
...o/opentelemetry/api/common/ExtendedAttributes.java 60.00% 1 Missing and 1 partial ⚠️
...lemetry/api/internal/InternalAttributeKeyImpl.java 90.47% 1 Missing and 1 partial ⚠️
... and 3 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6983      +/-   ##
============================================
- Coverage     89.99%   89.30%   -0.70%     
- Complexity     6601     6640      +39     
============================================
  Files           730      737       +7     
  Lines         19864    20065     +201     
  Branches       1956     1964       +8     
============================================
+ Hits          17877    17919      +42     
- Misses         1391     1534     +143     
- Partials        596      612      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


@Override
public Attributes getAttributes() {
return getExtendedAttributes().asAttributes();
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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);
Copy link
Member

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

Copy link
Member Author

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

Comment on lines 115 to +118
<T> LogRecordBuilder setAttribute(AttributeKey<T> key, T value);

/** TODO. */
default <T> LogRecordBuilder setAttribute(ExtendedAttributeKey<T> key, T value) {
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants