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

Add support for Sentry Kotlin Compiler Plugin #2695

Merged
merged 11 commits into from
May 26, 2023

Conversation

markushi
Copy link
Member

@markushi markushi commented May 4, 2023

📜 Description

Counterpart for Kotlin Compiler Plugin

💡 Motivation and Context

  • Adds compose support for automatic user interactions tracking
  • Add View Hierarchy support

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 57d1a71

@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 317.14 ms 342.80 ms 25.66 ms
Size 1.72 MiB 2.28 MiB 570.71 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
46b1782 387.72 ms 458.74 ms 71.02 ms
1707044 338.80 ms 384.79 ms 46.00 ms

App size

Revision Plain With Sentry Diff
46b1782 1.72 MiB 2.28 MiB 570.44 KiB
1707044 1.72 MiB 2.28 MiB 570.44 KiB

Previous results on branch: feat/compose-compiler-plugin

Startup times

Revision Plain With Sentry Diff
5ea803d 326.18 ms 359.36 ms 33.18 ms
ffa0ad4 289.42 ms 376.22 ms 86.80 ms
ea7fcbe 367.34 ms 414.72 ms 47.38 ms
775fa63 298.58 ms 388.15 ms 89.56 ms

App size

Revision Plain With Sentry Diff
5ea803d 1.72 MiB 2.28 MiB 570.71 KiB
ffa0ad4 1.72 MiB 2.28 MiB 565.75 KiB
ea7fcbe 1.72 MiB 2.26 MiB 553.74 KiB
775fa63 1.72 MiB 2.26 MiB 553.74 KiB

@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Patch coverage: 20.00% and project coverage change: -0.02 ⚠️

Comparison is base (a485ab0) 81.11% compared to head (826f763) 81.09%.

❗ Current head 826f763 differs from pull request most recent head 57d1a71. Consider uploading reports for the commit 57d1a71 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2695      +/-   ##
============================================
- Coverage     81.11%   81.09%   -0.02%     
  Complexity     4447     4447              
============================================
  Files           345      345              
  Lines         16394    16399       +5     
  Branches       2226     2226              
============================================
+ Hits          13298    13299       +1     
- Misses         2168     2172       +4     
  Partials        928      928              
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/SentryOptions.java 79.95% <20.00%> (-0.63%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@markushi markushi changed the title Add support for Sentry Compose Compiler Plugin Add support for Sentry Kotlin Compiler Plugin May 4, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

Code-wise this looks good, but I have few rather conceptual things:

  • When I tried integrating kotlin-compiler-plugin into the sample, I've faced a compilation error, because our sample uses Kotlin 1.8, and the compiler plugin seems to support only 1.8.20 onwards (after updating the kotlin version, it worked). The error was the same one as Kotlin 1.8.20 support drewhamilton/Poko#122. The question is if we want to support lower version of Kotlin, or we just say we support the latest one onwards?
  • I thought of moving SentryModifier and SentryComposeTracing to commonMain if possible, but I guess we could do it later when we start looking into compose-multiplatform support
  • Last thing - I've tried integrating the compiler plugin into our sample, but unfortunately I don't think it brings much value in the current state (unless I misunderstood how it all should work in the end). E.g. for our sample on the screenshot I've removed all of the custom SentryTraced wrappers and just left the compiler plugin do its job and it only inferred one tag (Landing). I guess we should rethink our instrumentation a bit for the view hierarchy case 😅
image

@markushi
Copy link
Member Author

Code-wise this looks good, but I have few rather conceptual things:

  • When I tried integrating kotlin-compiler-plugin into the sample, I've faced a compilation error, because our sample uses Kotlin 1.8, and the compiler plugin seems to support only 1.8.20 onwards (after updating the kotlin version, it worked). The error was the same one as Kotlin 1.8.20 support drewhamilton/Poko#122. The question is if we want to support lower version of Kotlin, or we just say we support the latest one onwards?
  • I thought of moving SentryModifier and SentryComposeTracing to commonMain if possible, but I guess we could do it later when we start looking into compose-multiplatform support
  • Last thing - I've tried integrating the compiler plugin into our sample, but unfortunately I don't think it brings much value in the current state (unless I misunderstood how it all should work in the end). E.g. for our sample on the screenshot I've removed all of the custom SentryTraced wrappers and just left the compiler plugin do its job and it only inferred one tag (Landing). I guess we should rethink our instrumentation a bit for the view hierarchy case 😅
image
  1. Yes, this is a known issue. The only alternative would be to release different versions of our kotlin compiler plugin and have a compatibility matrix, just like Jetpack Compose has, but as of now this would also mean having different versions of our AGP as well. I'd stick to the 1.8.20 requirement.
  2. Sounds good!
  3. This looks odd, there should definitely be more metadata in there. Let me have a closer look at this.

@romtsn
Copy link
Member

romtsn commented May 23, 2023

Yes, this is a known issue. The only alternative would be to release different versions of our kotlin compiler plugin and have a compatibility matrix, just like Jetpack Compose has, but as of now this would also mean having different versions of our AGP as well. I'd stick to the 1.8.20 requirement.

I guess we'll still have to maintain the compat matrix for the future versions of Kotlin, but yeah let's start with 1.8.20 as the initial version

@@ -2,6 +2,12 @@

## Unreleased

### Features
- Add support for Sentry Kotlin Compiler Plugin ([#2695](https://github.com/getsentry/sentry-java/pull/2695))
- In conjunction with our sentry-kotlin-compiler-plugin we improved Jetpack Compose support for
Copy link
Member

Choose a reason for hiding this comment

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

nit: Ideally here would be also a link to sentry-kotlin-compiler-plugin docs, but we can update it later ;)

Choose a reason for hiding this comment

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

I'd really appreciate a link to the compiler plugin docs as I've been meaning to try these new features and haven't been able to understand how to use the compiler plugin.

Copy link
Member

Choose a reason for hiding this comment

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

@tfcporciuncula they are in the making, but soon will be there :) getsentry/sentry-docs#7026

@markushi markushi merged commit 46e0307 into main May 26, 2023
@markushi markushi deleted the feat/compose-compiler-plugin branch May 26, 2023 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants