Remove core library desugaring from android lib conventions

[surbhiia]

As discussed in today's sig meeting about issue #682, I removed the added by default core library desugaring from android lib conventions.

Project assembles fine but ./gradlew check errors out on lintDebug task with 56 instances of requiring API level 24/26 or desugaring, as our minSdk = 21 :

We earlier used a @SuppressLint('NewApi') annotation to suppress such warning instead of creating a lint baseline file (as maintaining that is tedious) :

@RequiresApi might not be correct here as it does not necessarily need that API level but can do with desugaring which is what we're going with here.

So, I think we need to add @SuppressLint('NewApi')` annotation at all 56 places? Wanted to cross check before going forward with that :D

[marandaneto]

If you want to make desugaring optional (and keeping the same features), you'd need to do runtime checks and use alternative classes/methods eg:

if API >= X
  do this
else
  do that

if that's the case, we'd need to check the otel-java as well, see https://developer.android.com/studio/write/lint and checkDependencies = true, maybe the dependency uses stuff that requires desugaring and we are not aware of.

if desugaring is mandatory for this SDK, then it's just safe to suppress all linters.

[surbhiia]

Thanks @marandaneto for your feedback!

oTel-java dependencies do cause us to require desugaring for API levels below 26, like this issue that often comes up:
java.lang.NoSuchMethodError: No static method stringKey(Ljava/lang/String;)Lio/opentelemetry/api/common/AttributeKey;

Adding checkDependencies = true will increase the time it takes for the ./gradlew check command.

I think we can go with clearly documenting the requirement, keeping the responsibility of enabling desugaring to the apps if they're using lower API levels and suppressing these lint errors in our libraries. What do you think?

[LikeTheSalad]

Thank you for putting up this PR, @surbhiia!

if desugaring is mandatory for this SDK, then it's just safe to suppress all linters.

It's mandatory for projects with minSdk lower than 26, this is something we can't easily get away from since the Java upstream project makes use of a lot of Java 8 tools that are only available on Android 26+ (or lower but with corelibs).

Adding checkDependencies = true will increase the time it takes for the ./gradlew check command.

This shouldn't be necessary as the Java upstream project uses animal sniffer to ensure they don't use apis that aren't available in the corelibs.

I think we can go with clearly documenting the requirement, #682 (comment) they're using lower API levels and suppressing these lint errors in our libraries. What do you think?

I agree, although since we're going to suppress the "NewApi" lint error in this project, we should replace it with a similar approach, ideally the same as the Java upstream repo by implementing animal sniffer checks to ensure we don't add code that's not supported by the corelibs. The animal sniffer plugin doesn't currently support android lib projects though, which is why I created this PR to enable that support, so as soon as that code is released I'm planning to implement it here to ensure we stay compliant with android apis.

In the meantime, I think it's fine to suppress the "NewApi" lint checks, however, I'd wait until we have a replacement before releasing a new version of OTel Android to be fully sure that we're not going to break any consumer projects.

So, I think we need to add @SuppressLint('NewApi')` annotation at all 56 places?

While that will work, I would prefer to not use the annotation because it might be tedious having to remember it every time we are going to use apis that are available via the corelib. Instead I would try to add the suppression in the otel.android-library-conventions.gradle.kts file using the lint block. I haven't tested this, but probably something like this should work:

android {
   lint {
      disable += ['NewApi']
   }
}

Maybe that's not the exact way to set it up because I'm not sure if the examples are using kotlin DSL or groovy, but it gives the overall idea. If this works, it'd be nicer because we wouldn't have to add a lot of annotations and we also wouldn't need to create an XML file for lint rules.

[marandaneto]

Adding checkDependencies = true will increase the time it takes for the ./gradlew check command.

Well yes, but this is a linter that would prevent runtime crashes, if otel-java adds a method that is available only on eg Android API 30 and you don't do runtime checks either on the Java or Android side, the app will crash at runtime.
I'm not sure animal sniffer can detect that (because the dependency is a compiled jar already), if it is, then it's redundant I agree.

[marandaneto]

IMO an app that compiles successfully but requires a desugaring set otherwise it crashes at runtime and is always a bad call.
I understand changing that now would require some refactoring, just sharing my 2 cents :) - crashes will come because of that for sure.

[LikeTheSalad]

Well yes, but this is a linter that would prevent runtime crashes, if otel-java adds a method that is available only on eg Android API 30 and you don't do runtime checks either on the Java or Android side, the app will crash at runtime. I'm not sure animal sniffer can detect that (because the dependency is a compiled jar already), if it is, then it's redundant I agree.

Ah, we commented almost at the same time, @marandaneto! I think I might have addressed this in my comment, otel-java does check that they're not using unsupported android corelib apis with animal sniffer checks, so (at least from the side of OTel Java) we shouldn't need to worry about it.

IMO an app that compiles successfully but requires a desugaring set otherwise it crashes at runtime and is always a bad call.
I understand changing that now would require some refactoring, just sharing my 2 cents :) - crashes will come because of that for sure.

For sure, it's not ideal. The refactoring bit is complicated because it would have to be done outside of this project (in the upstream otel-java repo) and the amount of work vs the benefit we'd get out of it, considering that using corelibs is an option, makes it a tough sell.

[marandaneto]

@LikeTheSalad yeah I understand that.
I say that because of #701 (comment)

This is a runtime error, animal sniffer will catch it during the SDK build, but not when people's apps are built, so the issue persists.
This will only prevent not using newer APIs from now on, but it won't with the ones that are already being used and already leading to runtime crashes.

Right now if they don't have desugaring enabled, they will either:

1. read the docs and know they need it (this is not a common behavior) - leading to 2.
2. face a crash - leading to 3.
3. find out they need desugaring enabled.

As I've seen a few threads and issues related to not having desugaring enabled already, that proves that a subset of users will have a bad experience after installing the SDK or even after rolling out to production which is even worse.

[LikeTheSalad]

I say that because of #701 (comment)

Got it, I didn't see that in detail.

@surbhiia - regarding this part:

like this issue that often comes up:
java.lang.NoSuchMethodError: No static method stringKey(Ljava/lang/String;)Lio/opentelemetry/api/common/AttributeKey;

I'm a bit confused by it because it doesn't seem to be related to desugaring. Usually, desugaring-related issues show missing JDK classes or methods, though the method you mentioned should come from the OTel Java lib instead, so I'm wondering if that "NoSuchMethodError: No static method stringKey" message might come from another type of issue, such as the use of reflection in an obfuscated build?

[surbhiia]

@surbhiia - regarding this part:

like this issue that often comes up:
java.lang.NoSuchMethodError: No static method stringKey(Ljava/lang/String;)Lio/opentelemetry/api/common/AttributeKey;

I'm a bit confused by it because it doesn't seem to be related to desugaring. Usually, desugaring-related issues show missing JDK classes or methods, though the method you mentioned should come from the OTel Java lib instead, so I'm wondering if that "NoSuchMethodError: No static method stringKey" message might come from another type of issue, such as the use of reflection in an obfuscated build?

This thread described the issue nicely. I took another look at it - looks like it is an issue in dexing when desugaring is enabled. So, you're right, it is not directly related to missed desugaring.

Given that, I hope the following holds true:

otel-java does check that they're not using unsupported android corelib apis with animal sniffer checks, so (at least from the side of OTel Java) we shouldn't need to worry about it.

[surbhiia]

I would prefer to not use the annotation because it might be tedious having to remember it every time we are going to use apis that are available via the corelib. Instead I would try to add the suppression in the otel.android-library-conventions.gradle.kts file using the lint block.

Disabled NewApi lint check in android lib conventions. Thanks @LikeTheSalad , this was a quicker change :)

In the meantime, I think it's fine to suppress the "NewApi" lint checks, however, I'd wait until we have a replacement before releasing a new version of OTel Android to be fully sure that we're not going to break any consumer projects.

We can wait on merging this PR if we think we might have a release before we have chance to incorporate animal sniffer otherwise we can merge it now itself. :)

[surbhiia]

As I've seen a few threads and issues related to not having desugaring enabled already, that proves that a subset of users will have a bad experience after installing the SDK or even after rolling out to production which is even worse.

Can we perhaps add a non-missable callout regarding the desugaring requirement in the README. Hopefully that'll help. It's in the Getting Started section already but we can add a better heading to the desugaring note to call attention to it like "IMPORTANT REQUIREMENT".

Also thinking that it might be complex to incorporate runtime checks - we might not always have ways to support lower API levels everywhere, where we have newer features / APIs used. Also, graceful handling of just logging or preventing runtime crash might be problematic as app users would not know why certain functionality doesn't work as expected until they dig into the logs.

[marandaneto]

This is a simple example that would require:

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
	return replace(connection, () -> connection.getContentLengthLong());
} else {
	return -1
}

And the linter wouldn't need to be suppressed but its out of the scope of this PR, just sharing.

[LikeTheSalad]

That's right, I wasn't hoping for animal sniffer to be a replacement for these kinds of approaches which we should still use IMO. We do so already in some services like this one for example.

The example highlighted here is a special one because the class HttpUrlReplacements is meant to be used during bytecode instrumentation to replace existing calls to the original methods, so for this case, the method HttpUrlReplacements.replacementForContentLengthLong will only be used in apps that are already calling the original method getContentLengthLong(), so it implicitly means that their minSdk is at least 24, otherwise they'd get an AGP compilation error.

That being said, I'm not fully sure what would happen in this case when animal sniffer is involved, I'm assuming it will complain because the signature we're going to use is of API 21 and also because I don't think this method has nothing to do with the desugaring corelibs either, so what I can propose is to follow what @surbhiia mentioned earlier about waiting for the animal sniffer integration before removing the "NewApi" checks just to make sure it behaves the same way and wouldn't just ignore these types of scenarios which are a potential runtime crash in most cases.

[marandaneto]

makes sense.

[marandaneto]

I'm still not a big fan of silencing the linter in the hopes that animalsnifer (a 3rd party) would prevent all the cases that the linter would catch, afaik animal sniffer is always behind at least for new releases (its well maintained at least).
animalsnifer IMO is a complementary tool.
Approaches like this is way more efficient IMO, but it has a lil performance impact at runtime, its just an if though.
I'll let @LikeTheSalad do the green mark :D

[LikeTheSalad]

I'm still not a big fan of silencing the linter in the hopes that animalsnifer (a 3rd party) would prevent all the cases that the linter would catch, afaik animal sniffer is always behind at least for new releases (its well maintained at least). animalsnifer IMO is a complementary tool. Approaches like this is way more efficient IMO, but it has a lil performance impact at runtime, its just an if though. I'll let @LikeTheSalad do the green mark :D

I think we should be pretty confident about animal sniffer capabilities before making the switch, so as I mentioned here, we should probably wait for it to be implemented to make sure it covers the same cases as the "NewApi" lint checks.

afaik animal sniffer is always behind at least for new releases (its well maintained at least).

Animal sniffer itself doesn't do the checks, it relies on "signatures" that contain the types and methods that are "allowed". You're right that not all the signatures are up-to-date as soon as a new release is made, this is actually an issue we had in the past where google released a new corelib version with some changes we needed but the animal sniffer signature we were using for corelibs wasn't up-to-date. However, we later found out that we could create our own signatures by using google's corelib artifact directly, which allows us to stay updated right away. So there's that workaround if it helps.

Ultimately, I would prefer to use native android checks as we do right now, but unfortunately it throws these false-positives that are very confusing to users with minSdk >= 26 (and rightfully so) so that's why I'm willing to give animal sniffer a try, which I hope it can do the same but without enforcing desugaring when it's not needed. Actually, to be honest I'd prefer we wouldn't have to add support to apis < 26 😅 but I know that's not feasible for now, so I hope people understand that they need to do these extra steps of configuring corelib desugaring when targeting apis < 26, though not when >= 26, that doesn't make sense.