Json facade for marshalling

[Hazer]

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

1. Instead of creating an instances of JsonFactory and JsonGenerator from Jackson, I made them interfaces
2. Then there's a JsonFactoryRuntimeClasspathLocator, that class tries to find the class with name io.sentry.marshaller.json.factory.JsonFactoryImpl in the classpath
3. It then creates a static reference to this factory and uses it later to create a new instance of JsonGenerator. At this point, JsonGenerator may be Jackson, Gson, whatever.
4. To support another Json library you just need to create a class implementing JsonGenerator and a
   io.sentry.marshaller.json.factory.JsonFactoryImpl to create your JsonGenerator class.

Discussion

Factory class loading

My first implementation actually had JacksonFactory and GsonFactory, 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 both JsonFactory implementations that way, using the same JsonFactoryImpl classname 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 to JsonFactorys 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 JacksonFactory and 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:

-keep class io.sentry.marshaller.json.factory.JsonFactoryImpl

Thanks

[bretthoerner]

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?

[Hazer]

@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.

[bretthoerner]

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. :)

[Hazer]

@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 :)

[iamareebjamal]

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

[friederbluemle]

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!

[bruno-garcia]

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?