Add experimental support for log extended attributes #7123

jack-berg · 2025-02-19T23:56:19Z

Supersedes #6983. This PR moves all the experimental stuff to opentelemetry-api-incubator, and significantly reshuffles things to make it all work while retaining only a compileOnly dependency on the incubator.

jack-berg · 2025-02-20T00:00:46Z

api/all/src/main/java/io/opentelemetry/api/common/AttributeKey.java

+  // TODO (jack-berg): uncomment when extended attributes are promoted from incubator to API
+  // default ExtendedAttributeKey<T> asExtendedAttributeKey() {
+  //  return InternalAttributeKeyImpl.toExtendedAttributeKey(this);
+  // }


This is a big loss. In the original prototype, I had a helper method to directly on AttributeKey for converting to ExtendedAttributeKey. This allowed the AttributeKey to internally keep a cached copy of the corresponding ExtendedAttributeKey, making conversions in both directions very cheap.

As long as ExtendedAttributes are experimental, we can't have this API. This means we need to a different strategy for converting / caching. I've opted for keeping a static ConcurrentHashMap cache in InternalExtendedAttributeKey. The problem is that the map can grow unbounded if a user puts high cardinality data into attribute keys. Not as elegant as caching directly in AttributeKey implementation.

Per a conversation in a java sig (several weeks ago I think), performance isn't a priority while this is experimental. Therefore, I've removed the unbounded cache for now.

jack-berg · 2025-02-20T00:02:00Z

api/incubator/src/main/java/io/opentelemetry/api/incubator/common/ExtendedAttributeType.java

+  LONG_ARRAY,
+  DOUBLE_ARRAY,
+  // Extended types unique to ExtendedAttributes
+  EXTENDED_ATTRIBUTES;


Dropped arrays of maps for now, per this comment: Sketch out ExtendedAttributes #6983 (comment)

Renamed from MAP -> EXTENDED_ATTRIBUTES per this comment: Sketch out ExtendedAttributes #6983 (comment)

Should we consider adding support for byte arrays?

I don't think it has to be now. Users will base64 encode or json stuff anyway. 🤷🏻

Yeah users will always have that ability, but having direct representation for a type a signal that there is a recommended way of representing the data. This encourages more consistency than being opinionated, which will produce a bit of a free for all in terms of what instrumentations produce and what backends receive.

Still, I'm happy to punt on these questions for now and let semantic conventions develop more guidance.

api/incubator/src/main/java/io/opentelemetry/api/incubator/common/ExtendedAttributes.java

jack-berg · 2025-02-20T00:03:17Z

api/incubator/src/main/java/io/opentelemetry/api/incubator/common/ExtendedAttributeKey.java

+
+  /** Return an ExtendedAttributeKey equivalent to the {@code attributeKey}. */
+  // TODO (jack-berg): remove once AttributeKey.asExtendedAttributeKey is available
+  static <T> ExtendedAttributeKey<T> fromAttributeKey(AttributeKey<T> attributeKey) {


Per this comment, the conversion from AttributeKey -> ExtendedAttributeKey unfortunately lives on ExtendedAttributeKey instead of on AttributeKey.

That totally seems like the kind of thing that will take some usage to determine if/how annoying it might be.

Its preferable to live on AttributeKey from a performance perspective, since the AttributeKey implementation can cache the equivalent ExtendedAttributeKey reference.

Once its available on AttributeKey, it could here as well as syntactic sugar. This static implementation could just call attributeKey.asExtendedAttributeKey() to obtain the cached copy.

jack-berg · 2025-02-20T00:04:41Z

.../src/main/java/io/opentelemetry/api/incubator/internal/InternalExtendedAttributeKeyImpl.java

+public final class InternalExtendedAttributeKeyImpl<T> implements ExtendedAttributeKey<T> {
+
+  private static final ConcurrentHashMap<AttributeKey<?>, ExtendedAttributeKey<?>>
+      ATTRIBUTE_KEY_CACHE = new ConcurrentHashMap<>();


Per this comment, keeping an unbounded cache mapping AttributeKey -> ExtendedAttributeKey.

For converting from ExtendedAttributeKey -> AttributeKey, we keep a reference to the AttributeKey as a field on ExtendedAttributeKey. See line 31.

jack-berg · 2025-02-20T00:05:24Z

...ubator/src/test/java/io/opentelemetry/api/incubator/logs/ExtendedLogsBridgeApiUsageTest.java

+
+  @Test
+  @SuppressWarnings("SystemOut")
+  void extendedAttributesUsage() {


The tests in this file demonstrate key usage patterns.

jack-berg · 2025-02-20T00:06:12Z

...ers/otlp/common/src/main/java/io/opentelemetry/exporter/internal/otlp/logs/LogMarshaler.java

+    int attributeSize =
+        INCUBATOR_AVAILABLE
+            ? IncubatingUtil.extendedAttributesSize(logRecordData)
+            : logRecordData.getAttributes().size();


Jumping through hoops so serialization logic forks based on whether opentelemetry-api-incubator (and ExtendedAttributes) is / isn't available.

codecov · 2025-02-20T00:15:28Z

Codecov Report

Attention: Patch coverage is 74.13043% with 119 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (9cb6365) to head (2ff9076).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...pi/incubator/common/ExtendedAttributesBuilder.java	41.66%	12 Missing and 9 partials ⚠️
...p/ExtendedAttributeKeyValueStatelessMarshaler.java	76.54%	11 Missing and 8 partials ⚠️
...tor/internal/InternalExtendedAttributeKeyImpl.java	81.42%	7 Missing and 6 partials ⚠️
...emetry/sdk/logs/ExtendedSdkReadWriteLogRecord.java	64.70%	9 Missing and 3 partials ⚠️
...elemetry/sdk/logs/ExtendedSdkLogRecordBuilder.java	60.71%	5 Missing and 6 partials ⚠️
...lemetry/exporter/internal/otlp/IncubatingUtil.java	75.60%	5 Missing and 5 partials ⚠️
...ntelemetry/sdk/internal/ExtendedAttributesMap.java	61.90%	7 Missing and 1 partial ⚠️
...y/api/incubator/logs/ExtendedLogRecordBuilder.java	50.00%	4 Missing and 2 partials ⚠️
...r/common/ArrayBackedExtendedAttributesBuilder.java	85.71%	2 Missing and 3 partials ⚠️
.../sdk/logs/internal/ExtendedReadWriteLogRecord.java	0.00%	5 Missing ⚠️
... and 4 more

❌ Your patch status has failed because the patch coverage (74.13%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7123      +/-   ##
============================================
- Coverage     89.92%   89.62%   -0.30%     
- Complexity     6721     6864     +143     
============================================
  Files           765      780      +15     
  Lines         20277    20715     +438     
  Branches       1985     2018      +33     
============================================
+ Hits          18234    18566     +332     
- Misses         1448     1511      +63     
- Partials        595      638      +43

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

...mon/src/test/java/io/opentelemetry/exporter/internal/otlp/logs/LogsRequestMarshalerTest.java

sdk/common/src/main/java/io/opentelemetry/sdk/internal/ExtendedAttributesMap.java

...ters/logging-otlp/src/test/java/io/opentelemetry/exporter/logging/otlp/TestDataExporter.java

…y-java into experimental-extended-attributes

...bator/src/main/java/io/opentelemetry/api/incubator/common/ArrayBackedExtendedAttributes.java

breedx-splk · 2025-03-21T17:37:57Z

...rc/main/java/io/opentelemetry/api/incubator/common/ArrayBackedExtendedAttributesBuilder.java

+    if (data.size() == 2 && data.get(0) != null) {
+      return new ArrayBackedExtendedAttributes(data.toArray());
+    }
+    return ArrayBackedExtendedAttributes.sortAndFilterToAttributes(data.toArray());


The sorting feels like part of the building, so what about moving the method into this builder class instead?

Yeah this whole function is essential a named constructor, since the constructor itself was private.

For what its worth, this whole class is a "copy / paste / modify" of ArrayBackedAttributes, so I didn't look too critically at the implementation.

What do you think about a followup PR where I cleanup minor pattern quirks like this in both ArrayBackedAttributes and ExtendedArrayBackedAttributes?

.../src/main/java/io/opentelemetry/api/incubator/internal/InternalExtendedAttributeKeyImpl.java

api/incubator/src/main/java/io/opentelemetry/api/incubator/logs/ExtendedLogRecordBuilder.java

api/incubator/src/test/java/io/opentelemetry/api/incubator/common/ExtendedAttributesTest.java

...ubator/src/test/java/io/opentelemetry/api/incubator/logs/ExtendedLogsBridgeApiUsageTest.java

...common/src/main/java/io/opentelemetry/exporter/internal/otlp/logs/LogStatelessMarshaler.java

sdk/common/src/main/java/io/opentelemetry/sdk/internal/ExtendedAttributesMap.java

sdk/logs/src/main/java/io/opentelemetry/sdk/logs/ExtendedSdkLogRecordData.java

breedx-splk

Looking really good. I'm a bit surprised and bummed that it required this much code, but with the incubator and explicit extension separation, it seems like maybe there's no great way around it.

I left a few comments, but I think this is close to being a good first pass. Thanks @jack-berg !!!

Next question, of course, is when do we get it out of incubator? 😅

jack-berg · 2025-03-25T16:51:06Z

I'm a bit surprised and bummed that it required this much code

Same. I think its a tradeoff between more code and better API ergonomics.

We could do it more succinctly, but the user API ergonomic story suffers. With this, we pay the price with more code to maintain, but help out the user with improved API ergonomics.

At least that's what I tell myself. 😅

breedx-splk

🚀

…y-java into experimental-extended-attributes

fandreuz · 2025-05-04T23:08:46Z

Hi @jack-berg, I'm working on open-telemetry/opentelemetry-java-instrumentation#8354 and I'm basing my changes on the new incubating extended attributes API. Would you be interested in some usability feedback?

Also, I think there's no way to fully test log records carrying extended attributes at the moment, am I missing something? I could use the implicit conversion to normal attributes, but that scrapes away maps. Is this addition planned already? Can I help somehow?

jack-berg · 2025-05-05T15:11:19Z

Would you be interested in some usability feedback?

Yes definitely! Care to open an issue to track?

Also, I think there's no way to fully test log records carrying extended attributes at the moment, am I missing something?

Yes there is a way. Its a bit obscure to follow the code (a consequence of the steps we take to separate to allow the SDK to work whether or not opentelemetry-api-incubator is present) but here's roughly how it works:

When opentelemetry-api-incubator is present, the implementation of SdkReadWriteLogRecord is ExtendedReadWriteLogRecord
Processors like BatchLogRecordProcessor obtain a LogRecordData instance to export by calling ReadWriteLogRecord#toLogRecordData. When the incubator is present, the implementation returns ExtendedLogRecordData.
ExtendedLogRecordData has an additional accessor, getExtendedAttributes for reading extended attributes

At the OTLP exporter level, the serializers are aware of the incubator and ExtendedLogRecordData, and if available, serialize complex attributes retaining the structure:

opentelemetry-java/exporters/otlp/common/src/main/java/io/opentelemetry/exporter/internal/otlp/logs/LogMarshaler.java

Lines 56 to 59 in 7cbcdd6

    
           KeyValueMarshaler[] attributeMarshalers = 
        
               INCUBATOR_AVAILABLE 
        
                   ? IncubatingUtil.createdExtendedAttributesMarhsalers(logRecordData) 
        
                   : KeyValueMarshaler.createForAttributes(logRecordData.getAttributes());

jack-berg added 4 commits December 28, 2024 13:10

Sketch out ExtendedAttributes

53d5439

Merge branch 'extended-attributes' into experimental-extended-attributes

ed799ef

Move to incubator, passing build

ea2155b

progress

b36e1ef

jack-berg requested a review from a team as a code owner February 19, 2025 23:56

jack-berg mentioned this pull request Feb 19, 2025

Sketch out ExtendedAttributes #6983

Closed

jack-berg commented Feb 20, 2025

View reviewed changes

api/incubator/src/main/java/io/opentelemetry/api/incubator/common/ExtendedAttributes.java Show resolved Hide resolved

jack-berg commented Feb 20, 2025

View reviewed changes

breedx-splk self-assigned this Mar 19, 2025

breedx-splk reviewed Mar 19, 2025

View reviewed changes

...mon/src/test/java/io/opentelemetry/exporter/internal/otlp/logs/LogsRequestMarshalerTest.java Show resolved Hide resolved

breedx-splk reviewed Mar 20, 2025

View reviewed changes

sdk/common/src/main/java/io/opentelemetry/sdk/internal/ExtendedAttributesMap.java Outdated Show resolved Hide resolved

breedx-splk reviewed Mar 20, 2025

View reviewed changes

...ters/logging-otlp/src/test/java/io/opentelemetry/exporter/logging/otlp/TestDataExporter.java Show resolved Hide resolved

jack-berg added 3 commits March 20, 2025 14:57

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

b24e012

…y-java into experimental-extended-attributes

Add unit tests

9b3f2ac

Drop unbounded cache

f313fd8