-
Notifications
You must be signed in to change notification settings - Fork 46
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
Making android min sdk configurable per auto-instrumentation subproject #147
Making android min sdk configurable per auto-instrumentation subproject #147
Conversation
Thanks @LikeTheSalad! I also tried it out in httpurlconnection module, it works perfectly! Just had a question about what we should call out in readme - |
It's probably the case that it works back to API 1 as you mentioned, however, I think it might be a little confusing to use that value since this tool relies on the OTel Java lib which has min API 21. So I would mention something like "The minimum supported Android SDK version is 21, though it will also instrument tools added in the Android SDK version 24 when running on devices with API level 24 and above". There's probably a better way to phrase it, but generally speaking, that would be my approach. |
Oh, I see. That makes sense! Will add that in the readme :) |
Another option is to throw an exception with this message when setting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool. I am not expecting to see a huge number of wildly differing instrumentations in the short term, but I do think about end-user ergonomics, and I wonder if having different minSdk versions for different components could cause user confusion (or at least burden).
Not a showstopper, but wanted to be thinking about it for later.
I think hopefully we should be able to keep minSdk = 21 that client apps can compile/run with for all components (even though the component itself could use otelAndroid.minSdk > 21 to compile with). And we can have a check to ensure that all components that compile with otelAndroid.minSdk > 21 need to test if it works correctly (compiles and runs correctly) on client apps with minSdk = 21 |
Yeah, ideally, and most likely, we wouldn't have to create too many of these add-ons with higher SDK version compatibility, especially because that should only happen when addressing Android OS's classes and I'm guessing third-party libs would be more often the targets of instrumentation. However, I cannot say we will never encounter a situation where we need to instrument OS's classes-related code added in newer API levels, for those cases, I think we should make sure that their READMEs explain this quite well to avoid confusion. The burden is still there though, but since these tools are optional, I think it's better to have that burden rather than enforcing our core lib api limitations on end-user's projects. This case is kinda like a hybrid one since it doesn't break the min SDK compatibility for 21 as @surbhiia explained, but it still supports code added past that version. |
Closes #138
This should allow to set the minimum Android SDK supported by a specific
auto-instrumentation
subproject like so:This will prevent animal sniffer from complaining about code references used by the auto-instrumentation project that aren't available neither in the Android SDK 21 (the current core lib min SDK supported) nor in the desugar lib.
The README of the auto-instrumentation subproject that changes this value should mention what's the minimum supported Android API level for the auto-instrumentation to work properly, this is because some auto-instrumentations might target code found on versions above 21 while still providing some functionality and being safe to run on devices with API level 21 (since the code from higher SDK versions would just be ignored as it wouldn't be found in devices with SDK version 21). In some other cases, the auto-instrumentation could cause issues when running on devices with the SDK version 21, so that's why it should be specified in the README what's the minimum version supported.