-
Notifications
You must be signed in to change notification settings - Fork 902
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.
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
OTLP exporter should tolerate instances of LogRecordData when incubator is present #7393
Conversation
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; |
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'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.
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 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
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.
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!
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 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.
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.
@laurit points out that ExtendedLogRecordData
is always available since its not in the incubator, so I think we're good here.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7393 +/- ##
=========================================
Coverage 89.76% 89.76%
- Complexity 6978 6981 +3
=========================================
Files 797 797
Lines 21165 21160 -5
Branches 2056 2056
=========================================
- Hits 18998 18994 -4
+ Misses 1505 1504 -1
Partials 662 662 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks @jack-berg appreciate it.
Fixes #7363