Skip to content

Conversation

trask
Copy link
Member

@trask trask commented Sep 23, 2025

@open-telemetry/android-approvers is this worrisome that removing these doesn't cause any failures?

Copy link

codecov bot commented Sep 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.12%. Comparing base (f3c8886) to head (8a4d66b).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7696   +/-   ##
=========================================
  Coverage     90.12%   90.12%           
  Complexity     7187     7187           
=========================================
  Files           814      814           
  Lines         21700    21700           
  Branches       2123     2123           
=========================================
  Hits          19557    19557           
  Misses         1477     1477           
  Partials        666      666           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@trask trask marked this pull request as ready for review September 23, 2025 17:22
@trask trask requested a review from a team as a code owner September 23, 2025 17:22
@breedx-splk
Copy link
Contributor

@open-telemetry/android-approvers is this worrisome that removing these doesn't cause any failures?

Worrisome for me, yeah. Is this possibly another scenario where the tests should be running but just aren't, and gradle doesn't complain about it?

@LikeTheSalad
Copy link
Contributor

@open-telemetry/android-approvers is this worrisome that removing these doesn't cause any failures?

The Android-related parts of animalsniffer for this repo are all in this file, which is then used in this convention one, none of which are changed in this PR, so I'm not worried that those would affect Android checks. They seem more related to concurrency tools that I'm not familiar with, which are probably impacted? But in terms of Android, I wouldn't worry about these changes.

@trask
Copy link
Member Author

trask commented Sep 25, 2025

TODO trask will try adding something that we know will fail on Android 23 to verify it's even running

@LikeTheSalad
Copy link
Contributor

TODO trask will try adding something that we know will fail on Android 23 to verify it's even running

A simple way to do this is to write some unsupported code into a module that uses the animalsniffer conventions and then run a gradle check on it.

For example, the File.toPath() method is not supported in Android < 26, not even with the corelib help, so by calling it somewhere in the code and running a check, it should raise an animalsniffer error.

I've just tested it by calling it from a class inside the common module. The changes I added are the following:

diff --git a/common/src/main/java/io/opentelemetry/common/ServiceLoaderComponentLoader.java b/common/src/main/java/io/opentelemetry/common/ServiceLoaderComponentLoader.java
index 5f2d0c58c..2176c6cc7 100644
--- a/common/src/main/java/io/opentelemetry/common/ServiceLoaderComponentLoader.java
+++ b/common/src/main/java/io/opentelemetry/common/ServiceLoaderComponentLoader.java
@@ -5,6 +5,7 @@
 
 package io.opentelemetry.common;
 
+import java.io.File;
 import java.util.ServiceLoader;
 
 class ServiceLoaderComponentLoader implements ComponentLoader {
@@ -17,6 +18,8 @@ class ServiceLoaderComponentLoader implements ComponentLoader {
 
   @Override
   public <T> Iterable<T> load(Class<T> spiClass) {
+    File file = new File("");
+    file.toPath();
     return ServiceLoader.load(spiClass, classLoader);
   }

Then I ran:

./gradlew :common:check

Which raised this issue in the console:

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':common:animalsnifferMain'.
> 1 AnimalSniffer violations were found in 1 files. See the report at: file:////[user-dir]/opentelemetry-java/common/build/reports/animalsniffer/main.text

One thing to note is that, iirc, this checks only our code, not the code from our dependencies. So if there's something from JCTools that is not supported by Android, for example, I think this tool won't let us know that. If that's the case, there's probably a way to create a custom animalsniffer task that also checks dependencies' code, though I haven't checked.

@jkwatson jkwatson merged commit 020f23f into open-telemetry:main Sep 26, 2025
48 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants