Conversation
… issue getsentry#461 Fixed json connector POM version definition Better naming and checkstyle fixes
|
Woah, that's a beast. I'll try to get a chance to look at it early next week. Can you remind me @Hazer do you want to this only to reduce the dependency size, or for another reason? |
|
@bretthoerner Dependencies size, but mostly because Android 65k methods limit makes things worse. Compile time, debugging and such. We are nearly hitting 65k in our project and we are soon to be integrating Sentry in our stack, we already use Gson, for its small footprint compared to Jackson, so if we added Sentry right now, we will get well past 65k and also now we will have 2 overlapping dependencies in our tree, which will hit our code review step, where we have a rule of no, specially big, dependencies overlapping. Our project have little room for exceptions in this regard. Also, it seems to me as a nice feature :) PS: Sorry for taking so long, I wasn't using Sentry since I changed workplace. |
|
Oh right, I forgot the 65k limit. Do you use Proguard or any other minifier? Just curious. I'll try to get some time to review this in a little while but we just had a kid so it may be a little bit. :) |
|
@bretthoerner Yeah Proguard, in the future migrating to R8 (same configs, but faster), but for most of our day-by-day build pipelines, we don't. It's a policy to reduce our build times until really needed. We use it only when we are almost hitting near the end of the development cycle. Also, we use Sentry in debug builds, not only for production, so we don't want to hit Proguard nor Multidex during debug. Fun fact: Last time we checked we had more deps in debug than production flavor, lots of debugging utils... No worries, I am just using the generated jars directly, for now, seems to be working fine, the build now seems to be failing to download a maven-plugin pom, I don't really know if I can do anything else to do it succeed, but everything else seems to be working. Take good care of your kid! I will look to contribute more if I can :) |
|
Guys, can you please look at this. Having 2 libraries(GSON and Jackson) in our codebase is not quite optimal. We considered moving to Jackson but overall it's not a better choice for us in terms of method count and size |
|
Hi @bruno-garcia - Has this already been addressed? Or the PR in its current form has been open for too long and is no longer applicable to the latest master branch? A quick note for the closing reason would be great. Thanks! |
|
Sorry for the lack of updates. 6.0.0 is coming out (alpha so far) and for now it's used gson so this PR is no longer relevant. There's no strategy to replace the serialize as of now though. Does gson work for you? |
5 participants
This pull request relates to issues #461 and #462.
Still work-in-progress but I want your feedback about the implementation and the dependency strategy. Specially @friederbluemle and @bretthoerner that already where taking a look at this issues.
Implementation overview
JsonFactoryandJsonGeneratorfrom Jackson, I made them interfacesJsonFactoryRuntimeClasspathLocator, that class tries to find the class with nameio.sentry.marshaller.json.factory.JsonFactoryImplin the classpathJsonGenerator. At this point,JsonGeneratormay be Jackson, Gson, whatever.JsonGeneratorand aio.sentry.marshaller.json.factory.JsonFactoryImplto create yourJsonGeneratorclass.Discussion
Factory class loading
My first implementation actually had
JacksonFactoryandGsonFactory, so I could have had both implementations in classpath that way. I found it weird, but thinking about testability now, the tests could evolve to run in bothJsonFactoryimplementations that way, using the sameJsonFactoryImplclassname we cannot use both at the same time, it's harder to test it, but gives some end-user stability, given that if the user has multiple dependencies pointing toJsonFactorys it will give a compile time error instead of a runtime error which looks to me the right way to behave.I would like to avoid using ServiceLoader as I saw some people reporting issues with it on Android.
GsonGenerator ByteArray serialization
The GsonGenerator I made cannot serialize
byte[]by now, and I don't know how Sentry backend expects this kind of data, maybe Base64 encoded?Dependencies and Publishing
Giving that it gets approval, it would be a breaking change migration or we will publish the main sentry lib with
JacksonFactoryand then people manually excludes the jackson-factory dependency when not wanted?I am not sure about any naming, especially for the new modules and artifacts.
Proguard
Must add new rule to proguard:
Thanks