No longer serialize static fields; use toString as fallback

[adinauer]

📜 Description

Skip static fields when serializing objects using reflection and use toString() as a fallback in case there were no fields on the object.

💡 Motivation and Context

Serializing something like Locale caused OOM as listing its static fields contained references to other locales that in turn referenced back (this is actually handled by our circular reference detection) causing an infinite loop. The real problem seems to come from each locale then also dragging in several other things that are costly to serialize.

Fixes #2301

💚 How did you test it?

Unit Tests

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• No breaking changes

🔮 Next steps

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	337.91 ms	371.75 ms	33.84 ms
Size	1.73 MiB	2.32 MiB	608.44 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
221a5df	350.61 ms	391.58 ms	40.97 ms
4ca1d7b	328.46 ms	368.22 ms	39.76 ms
54cebc8	331.12 ms	385.14 ms	54.02 ms
54cebc8	300.86 ms	341.43 ms	40.57 ms
2c5f172	310.20 ms	357.16 ms	46.96 ms
2c5f172	289.18 ms	307.56 ms	18.38 ms
7300956	324.20 ms	353.79 ms	29.58 ms
c5ccd8a	329.98 ms	365.52 ms	35.54 ms
d4087ee	278.00 ms	313.86 ms	35.86 ms
4ca1d7b	331.33 ms	335.78 ms	4.45 ms

App size

Revision	Plain	With Sentry	Diff
221a5df	1.73 MiB	2.29 MiB	581.39 KiB
4ca1d7b	1.73 MiB	2.29 MiB	579.88 KiB
54cebc8	1.73 MiB	2.29 MiB	579.43 KiB
54cebc8	1.73 MiB	2.29 MiB	579.43 KiB
2c5f172	1.73 MiB	2.29 MiB	580.10 KiB
2c5f172	1.73 MiB	2.29 MiB	580.10 KiB
7300956	1.73 MiB	2.29 MiB	578.69 KiB
c5ccd8a	1.74 MiB	2.33 MiB	607.44 KiB
d4087ee	1.73 MiB	2.29 MiB	579.50 KiB
4ca1d7b	1.73 MiB	2.29 MiB	579.88 KiB

Previous results on branch: fix/locale-serialization

Startup times

Revision	Plain	With Sentry	Diff
7b8722b	355.58 ms	386.42 ms	30.84 ms
2d20f2d	309.22 ms	351.43 ms	42.21 ms
f38142c	294.94 ms	370.86 ms	75.92 ms
97a2d6f	326.71 ms	369.06 ms	42.35 ms

App size

Revision	Plain	With Sentry	Diff
7b8722b	1.73 MiB	2.29 MiB	581.28 KiB
2d20f2d	1.73 MiB	2.32 MiB	607.79 KiB
f38142c	1.73 MiB	2.32 MiB	607.42 KiB
97a2d6f	1.73 MiB	2.32 MiB	607.82 KiB

[adinauer]

GSON handles this by having TypeHandlers for certain classes, amongst them is Locale. Maybe we should also handle Locale and others explicitly.

[stefanosiano]

I agree going with explicit Locale handling, using something like serializeLocale in JsonObjectSerializer, so we can avoid reflection, and falling back to toString in other cases.
I guess Gson treats things differently because it gives the user a way to provide their own serializers implementations. Our use case is not to let users serialize and deserialize custom objects in their own code, right?
If we want to achieve a really generic solution that can work with anything we could go with an annotation processor or with custom JsonSerializers and let the users create their own serializers and register them. But I'm not sure how common is to serialize custom objects, especially in ways where toString is not enough.

[adinauer]

@stefanosiano there's setSerializer on SentryOptions.

Only doing the conversions in JsonObjectSerializer doesn't help for complex objects etc., they need to be in JsonReflectionObjectSerializer. Having them in JsonObjectSerializer too is just an optimization.

Should we add them to both?
I've added more types in my last commit and I've also changed the toString to produce a String instead of a map with key toString.

[stefanosiano]

@adinauer yeah, i meant explicitly handling the Locale conversion, while keeping the changes you already made in JsonReflectionObjectSerializer
So we can optimize for simple Locale objects, and use reflection when it is nested inside another one

[codecov-commenter]

Codecov Report

Base: 80.09% // Head: 80.13% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (04ef56c) compared to base (f50fb16).
Patch coverage: 94.11% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2309      +/-   ##
============================================
+ Coverage     80.09%   80.13%   +0.03%     
- Complexity     3431     3443      +12     
============================================
  Files           242      242              
  Lines         12756    12789      +33     
  Branches       1703     1714      +11     
============================================
+ Hits          10217    10248      +31     
- Misses         1890     1891       +1     
- Partials        649      650       +1     

Impacted Files	Coverage Δ
...java/io/sentry/JsonReflectionObjectSerializer.java	92.00% <94.11%> (+0.95%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[adinauer]

I assume it's sufficient to duplicate the simpler serialization stuff into JsonObjectSerializer and keep the more complex ones (e.g. for Calendar) in JsonReflectionObjectSerializer @stefanosiano? Or do we prefer extracting the logic to some util and reusing it in both? Don't wanna micro optimize here. Maybe it's better to change this some more and just move most of the code into JsonReflectionObjectSerializer so we don't have all of it twice?

[adinauer]

I've updated the code.

[markushi]

LGTM!

[adinauer]

@stefanosiano do you want to take another look or can I go ahead and merge?

[stefanosiano]

2022L should be enough, shouldn't it?

[adinauer]

It didn't like that. I assume it's some mockito kotlin / java interop thing.