Skip to content
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

Jump to bottom

Remove core library desugaring from android lib conventions #701

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Remove core library desugaring from android lib conventions #701

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
9bffec0
Remove core library desugaring from android lib conventions
surbhiia Nov 19, 2024
6802e31
Remove NewApi lint check from android lib conventions
surbhiia Nov 21, 2024
dd61d80
Remove suppress new api annotation from HttpUrlReplacements.java
surbhiia Nov 21, 2024
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
3 changes: 1 addition & 2 deletions buildSrc/src/main/kotlin/otel.android-library-conventions.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ android {
// we rely on dependabot for dependency updates
disable.add("GradleDependency")
disable.add("AndroidGradlePluginVersion")
disable.add("NewApi")
}

compileOptions {
sourceCompatibility(javaVersion)
targetCompatibility(javaVersion)
isCoreLibraryDesugaringEnabled = true
}

kotlinOptions {
Expand All @@ -53,5 +53,4 @@ dependencies {
testRuntimeOnly(libs.findLibrary("junit-platform-launcher").get())
testImplementation(libs.findLibrary("opentelemetry-sdk-testing").get())
testImplementation(libs.findLibrary("androidx-junit").get())
coreLibraryDesugaring(libs.findLibrary("desugarJdkLibs").get())
}
1 change: 0 additions & 1 deletion .../java/io/opentelemetry/instrumentation/library/httpurlconnection/HttpUrlReplacements.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ public static int replacementForContentLength(URLConnection connection) {
return replace(connection, () -> connection.getContentLength());
}

@SuppressLint("NewApi")
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense.

public static long replacementForContentLengthLong(URLConnection connection) {
return replace(connection, () -> connection.getContentLengthLong());
}
Expand Down