Add Android API-friendliness checks #4505
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
instrumentation-api/build.gradle.kts
Outdated
|
|
||
| // these standard Java classes seem to be desugared properly | ||
| "java.lang.ClassValue", | ||
| "java.util.concurrent.CompletableFuture", |
There was a problem hiding this comment.
I'm not aware of this working on API21 still (how are you verifying it desugars properly?).
I think it's ok because we don't call any caffeine API using it though so that's the comment we should add I guess?
There was a problem hiding this comment.
how are you verifying it desugars properly?
I'm basing it on a very simple (probably too simple 😅 ) assumption -- if splunk-otel-android compiles correctly with instrumentation-api that uses those they're probably fine.
But it may as well be that those classes are just being referenced in those parts of caffeine that we don't call and it' the reason the whole thing works - I'll check that and update the comment.
There was a problem hiding this comment.
Thanks for clarifying - indeed that looks like API level 21
https://github.com/signalfx/splunk-otel-android/blob/main/splunk-otel-android/build.gradle
And if it's working it seems likely that it's because we don't call it indeed.
There was a problem hiding this comment.
Okay, I looked through all the ignored classes and it does seem that most of them come from not used code paths.
There are still several points to consider:
Unsafeis used in lots of places in caffeine - whether we're not really using it or it works properly on Android, it's hard to say. It seems to be okay though, so I left the ignore.LongAdderin caffeine2 is only present in unused paths; we're using it in ourSupportabilityMetricsthough. Not used unless somebody sets theotel.javaagent.debugproperty, which seems rather unlikely in Android.ClassValueis used inSpanNamesandClassNames- we're not using any of these in our Android repo, but it's not failing the compilation on desugaring problems so I decided to keep it in the ignores. I have no idea whether this actually works on Android though.
There was a problem hiding this comment.
I think it's quite dangerous to put this here, since you might use it, and if it is actually used, it will break Android compatibility. Same with any of these overrides.
There was a problem hiding this comment.
I think we can quite easily remove the ClassValue and LongAdder usage - for example, we could replace ClassValue with our own Cache (we should better compare these 2 in a benchmark first though).
I don't have any good idea about what to do with the caffeine classes though.
| } | ||
|
|
||
| dependencies { | ||
| add("signature", "com.toasttab.android:gummy-bears-api-21:0.3.0:coreLib@signature") |
There was a problem hiding this comment.
Should we call this conventions something like android or android-compatible?
There was a problem hiding this comment.
Sure, I don't mind - I used otel.animalsniffer-conventions because the SDK file is named the same way.
There was a problem hiding this comment.
android-compatibility? (and we can update in SDK repo too)
|
I added to tomorrow's SIG agenda to discuss if Caffeine is worth it for us given our particular constraints/issues around mrjars in the bootstrap class loader and android compatibility detection. |
|
I removed all of the ignores, and added reflective handling for |
instrumentation-api/build.gradle.kts
Outdated
| withType<AnimalSniffer>().configureEach { | ||
| // we catch NoClassDefFoundError when use caffeine3 is not available on classpath and fall back to caffeine2 | ||
| exclude("**/internal/shaded/caffeine3/**") | ||
|
|
||
| ignoreClasses = listOf( | ||
| // ignore shaded caffeine3 references | ||
| "io.opentelemetry.instrumentation.api.internal.shaded.caffeine3.cache.Caffeine", | ||
| "io.opentelemetry.instrumentation.api.internal.shaded.caffeine3.cache.Cache", | ||
|
|
||
| // these are being referenced by some caffeine2 classes, but we don't use them | ||
| "java.util.concurrent.CompletableFuture", | ||
| "java.util.concurrent.CompletionException", | ||
| "java.util.concurrent.CompletionStage", | ||
| "java.util.concurrent.ForkJoinPool", | ||
| "java.util.concurrent.ConcurrentHashMap", | ||
| // LongAdder is also referenced by our own SupportabilityMetrics; only if agent debug is enabled though | ||
| "java.util.concurrent.atomic.LongAdder", | ||
|
|
||
| // these standard Java classes seem to be desugared properly | ||
| "java.lang.ClassValue", | ||
| "sun.misc.Unsafe", | ||
| ) | ||
| } |
* Add Android API-friendliness checks * Improve comments * Remove ignores * Handle CompletionException * Spotless Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
Resolves #3913
I added animalsniffer checks to
instrumentation-api, OkHttp, gRPC and Apache HTTP client library instrumentations.instrumentation-apineeded a lot of additional ignores, mostly because of shaded caffeine2 - but they all currently seem to be desugared properly so it's all good, probably.