ExtendedAttributes issue with custom LogRecordData impl

[LikeTheSalad]

Describe the bug
This exception is thrown when a custom implementation of LogRecordData is used, and the extended attributes are enabled. This happens specifically when using disk-buffering, which has a custom LogRecordData implementation for when data is deserialized.

There's no issue if the extended attributes aren't enabled, though it seems like they become automatically enabled if the runtime classpath has the extended types available (for this particular case, I think it happens here), so there doesn't seem to be a way to disable them. In my case, it seems like the incubating library comes as a transitive one via the okhttp instrumentation.

Steps to reproduce

• Add the incubating library to a project.
• Export logs using a custom implementation of LogRecordData, such as the one provided by the disk-buffering lib.
• Try to send a log record.

I've modified OTel Android's demo app to reproduce this issue in this branch.

What did you expect to see?
No issues.

What did you see instead?
This stacktrace:

java.lang.IllegalArgumentException: logRecordData must be ExtendedLogRecordData
	at io.opentelemetry.exporter.internal.otlp.IncubatingUtil.getExtendedAttributes(IncubatingUtil.java:121)
	at io.opentelemetry.exporter.internal.otlp.IncubatingUtil.sizeExtendedAttributes(IncubatingUtil.java:110)
	at io.opentelemetry.exporter.internal.otlp.logs.LogStatelessMarshaler.getBinarySerializedSize(LogStatelessMarshaler.java:103)
	at io.opentelemetry.exporter.internal.otlp.logs.LogStatelessMarshaler.getBinarySerializedSize(LogStatelessMarshaler.java:26)
	at io.opentelemetry.exporter.internal.marshal.StatelessMarshalerUtil.sizeRepeatedMessageWithContext(StatelessMarshalerUtil.java:135)
	at io.opentelemetry.exporter.internal.otlp.logs.InstrumentationScopeLogsStatelessMarshaler.getBinarySerializedSize(InstrumentationScopeLogsStatelessMarshaler.java:55)
	at io.opentelemetry.exporter.internal.otlp.logs.InstrumentationScopeLogsStatelessMarshaler.getBinarySerializedSize(InstrumentationScopeLogsStatelessMarshaler.java:21)
	at io.opentelemetry.exporter.internal.marshal.StatelessMarshalerUtil$RepeatedElementPairSizeCalculator.accept(StatelessMarshalerUtil.java:263)
	at java.util.IdentityHashMap.forEach(IdentityHashMap.java:1351)
	at io.opentelemetry.exporter.internal.marshal.StatelessMarshalerUtil.sizeRepeatedMessageWithContext(StatelessMarshalerUtil.java:188)
	at io.opentelemetry.exporter.internal.otlp.logs.ResourceLogsStatelessMarshaler.getBinarySerializedSize(ResourceLogsStatelessMarshaler.java:67)
	at io.opentelemetry.exporter.internal.otlp.logs.ResourceLogsStatelessMarshaler.getBinarySerializedSize(ResourceLogsStatelessMarshaler.java:28)
	at io.opentelemetry.exporter.internal.marshal.StatelessMarshalerUtil$RepeatedElementPairSizeCalculator.accept(StatelessMarshalerUtil.java:263)
	at java.util.IdentityHashMap.forEach(IdentityHashMap.java:1351)
	at io.opentelemetry.exporter.internal.marshal.StatelessMarshalerUtil.sizeRepeatedMessageWithContext(StatelessMarshalerUtil.java:188)
	at io.opentelemetry.exporter.internal.otlp.logs.LowAllocationLogsRequestMarshaler.calculateSize(LowAllocationLogsRequestMarshaler.java:84)
	at io.opentelemetry.exporter.internal.otlp.logs.LowAllocationLogsRequestMarshaler.initialize(LowAllocationLogsRequestMarshaler.java:57)
	at io.opentelemetry.exporter.internal.otlp.logs.LogReusableDataMarshaler.export(LogReusableDataMarshaler.java:46)
	at io.opentelemetry.exporter.otlp.http.logs.OtlpHttpLogRecordExporter.export(OtlpHttpLogRecordExporter.java:81)
	at io.opentelemetry.contrib.disk.buffering.LogRecordFromDiskExporter$$ExternalSyntheticLambda0.apply(D8$$SyntheticClass:0)
	at io.opentelemetry.contrib.disk.buffering.internal.exporter.FromDiskExporterImpl.lambda$exportStoredBatch$0$io-opentelemetry-contrib-disk-buffering-internal-exporter-FromDiskExporterImpl(FromDiskExporterImpl.java:73)
	at io.opentelemetry.contrib.disk.buffering.internal.exporter.FromDiskExporterImpl$$ExternalSyntheticLambda0.apply(D8$$SyntheticClass:0)
	at io.opentelemetry.contrib.disk.buffering.internal.storage.files.ReadableFile.readAndProcess(ReadableFile.java:101)
	at io.opentelemetry.contrib.disk.buffering.internal.storage.Storage.readAndProcess(Storage.java:105)
	at io.opentelemetry.contrib.disk.buffering.internal.storage.Storage.readAndProcess(Storage.java:83)
	at io.opentelemetry.contrib.disk.buffering.internal.exporter.FromDiskExporterImpl.exportStoredBatch(FromDiskExporterImpl.java:61)
	at io.opentelemetry.contrib.disk.buffering.LogRecordFromDiskExporter.exportStoredBatch(LogRecordFromDiskExporter.java:41)
	at io.opentelemetry.android.features.diskbuffering.SignalFromDiskExporter.exportBatchOfLogs(SignalFromDiskExporter.kt:69)
	at io.opentelemetry.android.features.diskbuffering.SignalFromDiskExporter.exportBatchOfEach(SignalFromDiskExporter.kt:87)
	at io.opentelemetry.android.features.diskbuffering.scheduler.DefaultExportScheduler.onRun(DefaultExportScheduler.kt:28)
	at io.opentelemetry.android.internal.services.periodicwork.PeriodicRunnable.run(PeriodicRunnable.kt:24)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
	at java.lang.Thread.run(Thread.java:1012)

What version and what artifacts are you using?
Artifacts: opentelemetry-sdk, opentelemetry-exporter-otlp, opentelemetry-api-incubator
Version: v1.50.0, v1.50.0-alpha

[breedx-splk]

I wish I knew what the right approach to fix this was -- should we return empty extended attributes in the util class instead of throwing? Or should we make the disk-buffering code support the extended attributes, even while they're still incubating. 🤔

[LikeTheSalad]

I wish I knew what the right approach to fix this was -- should we return empty extended attributes in the util class instead of throwing? Or should we make the disk-buffering code support the extended attributes, even while they're still incubating. 🤔

It's also not too clear to me, tbh. For the mobile side, I think it makes sense to support extended attributes as soon as possible, as I believe those will be needed to properly use events. Apart from that, I think it's worth emphasizing that the issue will happen with any custom implementation of LogRecordData, which I'm aware is not likely that there will be too many of those out there 😄 but probably not zero, so I think it's worth considering implementing a fallback mechanism, given that the feature can get enabled without users being aware of it (as the incubating dependency can be brought into the runtime as a transitive dependency).

For the Android specific use-case, I think it makes sense to add support for extended attributes in disk-buffering, while they're incubating, especially if we think that this feature will remain in the incubating state for a while.

[bidetofevil]

I think there might be two issues here: 1) how we can modify the disk buffering code to support this; and 2) whether this is expected behaviour.

To me, enabling this globally in the OTLP exporter seems to imply that all the components in the workflow will have to necessarily support it too (or at least the component sending the log record to the export does). Is that the intention?

If it's not, another way to address this is by changing the way this is enabled: instead of using reflection to detect the class at runtime, make it a configurable explicitly by other means.

FWIW, the reflection usage makes it not compatible with the obfuscation done for Android apps. we can add an exclusion in the proguard file, but it's not foolproof and kind of seems inappropriate in the long run.

[laurit]

I would try changing INCUBATOR_AVAILABLE to INCUBATOR_AVAILABLE && log instanceof ExtendedLogRecordData.

FWIW, the reflection usage makes it not compatible with the obfuscation done for Android apps. we can add an exclusion in the proguard file, but it's not foolproof and kind of seems inappropriate in the long run.

As far as I remember proguard is pretty smart, it should also replace the class name used in Class.forName with the obfuscated name.