Skip to content

OTLP exporter should tolerate instances of LogRecordData when incubator is present #7393

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

Merged
jack-berg merged 3 commits into from
Jun 6, 2025
Merged

OTLP exporter should tolerate instances of LogRecordData when incubator is present #7393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d5a9dc1
OTLP exporter should tolerate instances of LogRecordData when incubat…
jack-berg Jun 5, 2025
f4335a7
Apply change to LogStatelessMarshaler
jack-berg Jun 5, 2025
8d4fc80
Fix build?
jack-berg Jun 5, 2025
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
17 changes: 17 additions & 0 deletions ...ers/otlp/common/src/main/java/io/opentelemetry/exporter/internal/otlp/IncubatingUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,28 @@
*/
public class IncubatingUtil {

private static final boolean INCUBATOR_AVAILABLE;

static {
boolean incubatorAvailable = false;
try {
Class.forName("io.opentelemetry.api.incubator.common.ExtendedAttributes");
incubatorAvailable = true;
} catch (ClassNotFoundException e) {
// Not available
}
INCUBATOR_AVAILABLE = incubatorAvailable;
}

private static final byte[] EMPTY_BYTES = new byte[0];
private static final KeyValueMarshaler[] EMPTY_REPEATED = new KeyValueMarshaler[0];

private IncubatingUtil() {}

public static boolean isExtendedLogRecordData(LogRecordData logRecordData) {
return INCUBATOR_AVAILABLE && logRecordData instanceof ExtendedLogRecordData;
Copy link
Copy Markdown
Member Author

@jack-berg jack-berg Jun 5, 2025

Choose a reason for hiding this comment

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

I'm nervous about this.

Recall that the test suites are set up that the main test suite intentionally doesn't include the incubator, to ensure that everything works without the incubator present.

The build was working locally when this was equal to return logRecordData instanceof ExtendedLogRecordData.. which suggests that instanceOf doesn't try to load classes, since if it did, it would load ExtendedLogRecordData and generate an exception.

I included this INCUBATOR_AVAILABLE check first in the hopes that if the incubator is not present ,the instanceOf check will never be reached and we'll never load the class. But not feeling confident about what the JVM will do here.

Copy link
Copy Markdown
Member

@trask trask Jun 5, 2025

Choose a reason for hiding this comment

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

maybe it's worth storing the ExtendedLogRecordData class object in a static final and using isAssignableFrom() to avoid the worry? I wouldn't expect perf to be much different

Copy link
Copy Markdown
Contributor

@breedx-splk breedx-splk Jun 5, 2025

Choose a reason for hiding this comment

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

Yeah weird I would very much expect this to be a problem. Weird. I did find a bit of prior art from way back when https://bugs.openjdk.org/browse/JDK-4354492...but I have no idea where things stand today. I tried an experiment with a silly class like this:

public class Tester {
  public static void main(String[] args) {
    Object x = "";
    if(x instanceof OpenTelemetry)
      System.out.println("interesting");
    else
      System.out.println("not interesting");
  }
}

and running this like such, yields an exception:

$ java -cp sdk/testing/build/classes/java/main io.opentelemetry.sdk.testing.Tester
Exception in thread "main" java.lang.NoClassDefFoundError: io/opentelemetry/api/OpenTelemetry
	at io.opentelemetry.sdk.testing.Tester.main(Tester.java:8)
Caused by: java.lang.ClassNotFoundException: io.opentelemetry.api.OpenTelemetry
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
	... 1 more

If I change the if statement to if(false && x instanceof OpenTelemetry) and recompile and re-run, it just runs without exception! Neat!

Copy link
Copy Markdown
Contributor

@breedx-splk breedx-splk Jun 5, 2025

Choose a reason for hiding this comment

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

I think maybe a comment for our future selves that says something like "this is ok, because the instanceof is only evaluated when the incubator is present, so the class can be found" or something, so we remember.

Copy link
Copy Markdown
Member Author

@jack-berg jack-berg Jun 6, 2025

Choose a reason for hiding this comment

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

@laurit points out that ExtendedLogRecordData is always available since its not in the incubator, so I think we're good here.

}

@SuppressWarnings("AvoidObjectArrays")
public static KeyValueMarshaler[] createdExtendedAttributesMarhsalers(
LogRecordData logRecordData) {
Expand Down
17 changes: 2 additions & 15 deletions .../otlp/common/src/main/java/io/opentelemetry/exporter/internal/otlp/logs/LogMarshaler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,6 @@
import javax.annotation.Nullable;

final class LogMarshaler extends MarshalerWithSize {
private static final boolean INCUBATOR_AVAILABLE;

static {
boolean incubatorAvailable = false;
try {
Class.forName("io.opentelemetry.api.incubator.common.ExtendedAttributes");
incubatorAvailable = true;
} catch (ClassNotFoundException e) {
// Not available
}
INCUBATOR_AVAILABLE = incubatorAvailable;
}

private static final String INVALID_TRACE_ID = TraceId.getInvalid();
private static final String INVALID_SPAN_ID = SpanId.getInvalid();

Expand All @@ -54,12 +41,12 @@ final class LogMarshaler extends MarshalerWithSize {

static LogMarshaler create(LogRecordData logRecordData) {
KeyValueMarshaler[] attributeMarshalers =
INCUBATOR_AVAILABLE
IncubatingUtil.isExtendedLogRecordData(logRecordData)
? IncubatingUtil.createdExtendedAttributesMarhsalers(logRecordData)
: KeyValueMarshaler.createForAttributes(logRecordData.getAttributes());

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

Expand Down
16 changes: 2 additions & 14 deletions ...mon/src/main/java/io/opentelemetry/exporter/internal/otlp/logs/LogStatelessMarshaler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,6 @@ final class LogStatelessMarshaler implements StatelessMarshaler<LogRecordData> {

private static final String INVALID_TRACE_ID = TraceId.getInvalid();
private static final String INVALID_SPAN_ID = SpanId.getInvalid();
private static final boolean INCUBATOR_AVAILABLE;

static {
boolean incubatorAvailable = false;
try {
Class.forName("io.opentelemetry.api.incubator.common.ExtendedAttributes");
incubatorAvailable = true;
} catch (ClassNotFoundException e) {
// Not available
}
INCUBATOR_AVAILABLE = incubatorAvailable;
}

static final LogStatelessMarshaler INSTANCE = new LogStatelessMarshaler();

Expand All @@ -56,7 +44,7 @@ public void writeTo(Serializer output, LogRecordData log, MarshalerContext conte
}

int droppedAttributesCount;
if (INCUBATOR_AVAILABLE) {
if (IncubatingUtil.isExtendedLogRecordData(log)) {
IncubatingUtil.serializeExtendedAttributes(output, log, context);
droppedAttributesCount =
log.getTotalAttributeCount() - IncubatingUtil.extendedAttributesSize(log);
Expand Down Expand Up @@ -99,7 +87,7 @@ public int getBinarySerializedSize(LogRecordData log, MarshalerContext context)
StatelessMarshalerUtil.sizeMessageWithContext(
LogRecord.BODY, log.getBodyValue(), AnyValueStatelessMarshaler.INSTANCE, context);
}
if (INCUBATOR_AVAILABLE) {
if (IncubatingUtil.isExtendedLogRecordData(log)) {
size += IncubatingUtil.sizeExtendedAttributes(log, context);

int droppedAttributesCount =
Expand Down
16 changes: 16 additions & 0 deletions ...java/io/opentelemetry/exporter/internal/otlp/logs/LogsRequestMarshalerIncubatingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.exporter.internal.otlp.logs;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;

import com.google.protobuf.ByteString;
import com.google.protobuf.InvalidProtocolBufferException;
Expand Down Expand Up @@ -34,6 +35,7 @@
import io.opentelemetry.sdk.common.InstrumentationScopeInfo;
import io.opentelemetry.sdk.logs.data.LogRecordData;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.testing.logs.TestLogRecordData;
import io.opentelemetry.sdk.testing.logs.internal.TestExtendedLogRecordData;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
Expand All @@ -56,6 +58,20 @@ class LogsRequestMarshalerIncubatingTest {
private static final String EVENT_NAME = "hello";
private static final String BODY = "Hello world from this log...";

// Marshalers should not throw if the incubator is on the class path, but are called with
// LogRecordData instances which are not ExtendedLogRecordData.
// See: https://github.com/open-telemetry/opentelemetry-java/issues/7363
@ParameterizedTest
@EnumSource(MarshalerSource.class)
void toProtoLogRecord_NotExtendedLogRecordData_DoesNotThrow(MarshalerSource marshalerSource) {
assertThatCode(
() ->
parse(
LogRecord.getDefaultInstance(),
marshalerSource.create(TestLogRecordData.builder().build())))
.doesNotThrowAnyException();
}

@ParameterizedTest
@EnumSource(MarshalerSource.class)
void toProtoLogRecord(MarshalerSource marshalerSource) {
Expand Down
Loading