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.

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

JaCoCo coverage metrics changed by enabling Sentry tracingInstrumentation #763

Open
cstavitsky opened this issue Sep 10, 2024 · 14 comments
Open

Comments

@cstavitsky
Copy link

cstavitsky commented Sep 10, 2024

Integration

sentry-android

Build System

Gradle

AGP Version

8.5.0

Proguard

Enabled

Version

7.14.0

Steps to Reproduce

(opening on behalf of a customer)

  1. Run unit test suite, measure coverage with JaCoCo without Sentry instrumented [customer is using Jacoco 0.8.7]
  2. Instrument Sentry, using the default Sentry gradle plugin settings [waiting to confirm which version of AGP customer uses], and rerun unit test suite. Coverage of the unit test suite drops by 15%:

Customer:
The total # of lines seen by Jacoco is approx. the same, but we see a large shift from "covered lines" <--> "missed lines"
Jacoco includes all the classes as part of the "Sessions" in the report, but I dont see any new Sentry-related files classes there, at least nothing with sentry in the package, so it might be something else going on. It's a little tricky for me to more interesting analysis with my current setup (i.e. look for patterns of what classes/lines are no longer covered).

  1. disable tracingInstrumentation per https://docs.sentry.io/platforms/android/troubleshooting/#auto-instrumentation-issues. Rerun unit test suite, it is back to the same coverage number as in step 1

Customer:
Confirming that adding tracingInstrumentation { enabled = false } to our gradle setup fixes the coverage issue

Expected Result

Expected result is that default Gradle tracingInstrumentation should not affect test coverage.

Actual Result

Unit test coverage drops by 15%, which we do not want. Is there an alternative to keep coverage normal, other than disabling tracingInstrumentation?

@LeonRa
Copy link

LeonRa commented Sep 11, 2024

Our AGP version is 8.5.0 and our Jacoco version is 0.8.7

@romtsn romtsn transferred this issue from getsentry/sentry-java Sep 11, 2024
@cstavitsky
Copy link
Author

Additional context from the customer:

I tried seeing how the coverage changes when disabling the file i/o, like in the docs, and it looks like the regression is still present in that case. I also checked the same thing for the other enums present (so, COMPOSE, DATABASE, and OKHTTP) and the regression was present in each. Not sure if that's helpful, but it looks like for now we have to disable it in the entirety to not see the coverage regression.
I'm still working on understanding more about what classes / coverage is actually being affected.

@LeonRa
Copy link

LeonRa commented Sep 13, 2024

We bumped Jacoco to its latest version (0.8.12) but unfortunately it still looks like enabling instrumentation drops our coverage by over 10%

@romtsn
Copy link
Member

romtsn commented Sep 16, 2024

hi, I've taken a look at this, and it seems like an issue is on the AGP's side: https://issuetracker.google.com/issues/233251847. Unfortunately, there's no fix yet, but the suggested workaround looks reasonable to me.

Do you use a custom task for jacoco? If so, I think the workaround should be straightforward to apply. Could you also share your jacoco task/setup, so we could think of applying the workaround automatically on our side?

I'm also not sure if it's possible to fix on the AGP's side, as it seems they are supplying the classpath to the unit-test task after bytecode transformations (which is correct), but then the very same classpath has to be used for jacoco as well.

@romtsn romtsn moved this from Needs Discussion to Needs Investigation in Mobile & Cross Platform SDK Sep 17, 2024
@romtsn romtsn moved this from Needs Investigation to Needs Discussion in Mobile & Cross Platform SDK Sep 17, 2024
@stefanosiano stefanosiano moved this from Needs Discussion to Needs More Information in Mobile & Cross Platform SDK Sep 18, 2024
@getsantry getsantry bot moved this to Waiting for: Community in GitHub Issues with 👀 3 Sep 18, 2024
@LeonRa
Copy link

LeonRa commented Sep 20, 2024

Thanks for finding that @romtsn! Just finished some testing with that workaround and the numbers definitely look a lot better. We're still losing about 3% in coverage in our app module, as opposed to the 14% we were losing before, so this workaround appears to have some shortcomings

@getsantry getsantry bot moved this from Waiting for: Community to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 20, 2024
@romtsn
Copy link
Member

romtsn commented Sep 20, 2024

@LeonRa could you share how you applied the workaround? Did you only replace your kotlin classes with the asm_transformed ones or also java classes?

@LeonRa
Copy link

LeonRa commented Sep 20, 2024

Only replaced the Kotlin classes as we're 100% Kotlin, unless that's a mistake and I should replace the Java ones as well? I will call out that the branch with incorrect coverage is the Android app module one, ins which the suggestion is to only map the Kotlin intermediates. There is no coverage loss in our non-app modules with this setup

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 20, 2024
@LeonRa
Copy link

LeonRa commented Sep 20, 2024

Workaround was applied as the following in our custom Jacoco class:

val buildDir = project.layout.buildDirectory.asFile.get().path
val excludes =
  Action<ConfigurableFileTree> {
    exclude(ANDROID_COVERAGE_EXCLUSIONS)
    exclude(ANDROID_HILT_EXCLUSIONS)
  }
val classesFileTree: ConfigurableFileTree =
  if (project.plugins.hasPlugin("com.android.application")) {
    project.fileTree(
      "$buildDir/intermediates/classes/${variant.name}/transform${variant.name.capitalize()}ClassesWithAsm/dirs",
      excludes
    )
  } else {
    project.fileTree("$buildDir/tmp/kotlin-classes/${variant.name}", excludes)
  }

@romtsn
Copy link
Member

romtsn commented Sep 23, 2024

@LeonRa yeah that looks legit. Do you notice any patterns in those files that lose coverage?

I see from my tests that the $buildDir/intermediates/classes/${variant.name}/transform${variant.name.capitalize()}ClassesWithAsm folder contains two subfolders: dirs and jars - is that also the case for you? I also assume that you're already excluding R.classes from coverage (coming from that jars folder), right?

@LeonRa
Copy link

LeonRa commented Sep 23, 2024

Do you notice any patterns in those files that lose coverage?

Unfortunately I can't say that I see any patterns in regards to what's losing coverage. The one thing I will note is that I'm seeing is a large increase in the number of branches and instructions, which increases the size of the denominator, hence decreasing the coverage. The numerator does shift as well, but not by nearly as much. For example, we have

  • Before enabling Sentry and applying the above fix: 1,294,544 of 1,889,317 instructions, 82,677 of 103,958 branches
  • After enabling Sentry and applying the fix: 1,401,873 of 2,033,053 instructions, 95,173 of 118,677 branches

I see from my tests that the $buildDir/intermediates/classes/${variant.name}/transform${variant.name.capitalize()}ClassesWithAsm folder contains two subfolders: dirs and jars - is that also the case for you? I also assume that you're already excluding R.classes from coverage (coming from that jars folder), right?

Yup, I see both folders on our end and I verified that that 0.jar (the only jar) only contains only R class files. We do exclude these and a couple of other things, specifically the following:

  • "**/R.class"
  • "**/R\$*.class"
  • "**/BuildConfig.*"
  • "**/Manifest*.*"
  • "**/*_Hilt*.class"
  • "**/Hilt_*.class"

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 23, 2024
@LeonRa
Copy link

LeonRa commented Sep 23, 2024

Did some more exploration today and, for the sake of sanity, disable the Sentry instrumentation while keeping the fix in place. This has the following results:

  • 1,404,600 of 2,036,846 instructions, 95,355 of 118,924 branches

The difference is stark and, further, subtracting these out shows that enabling Sentry instrumentation results in a drop of 1,066 instructions and 65 branches, leading me to believe that the underlying problem is actually the workaround in this case.

@romtsn
Copy link
Member

romtsn commented Sep 23, 2024

leading me to believe that the underlying problem is actually the workaround in this case.

that's probably because you have some other gradle plugins that do bytecode instrumentation (like firebase performance or dagger hilt, etc.), I guess they were not included in your baseline, hence the increase in instructions/branches.

Tbh, I'm not sure if there's a proper fix right now, other than disabling instrumentation for tests altogether. On the other hand, it seems like a correct thing to do to test your already instrumented code, also Google does it for their internal tests (this will return already instrumented classes).

@LeonRa
Copy link

LeonRa commented Sep 26, 2024

Thanks for your help here @romtsn! Happy to confirm that I've gotten things to work by adding a few extra exclusions on top of the workaround above:

  • **/databinding/*.class
  • dagger/hilt/internal/**/*.class
  • hilt_aggregated_deps/*.class

@getsantry getsantry bot moved this from Waiting for: Community to Waiting for: Product Owner in GitHub Issues with 👀 3 Sep 26, 2024
@romtsn
Copy link
Member

romtsn commented Sep 26, 2024

@LeonRa that's great to hear! I will formalize this and put onto our troubleshooting page, as I doubt we can do something on the plugin's side here

@romtsn romtsn moved this from Needs More Information to Todo in Mobile & Cross Platform SDK Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: Todo
Development

No branches or pull requests

6 participants