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

Fix json parsing of nullable/empty fields #2968

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Oct 5, 2023

📜 Description

  • Renames nextList to nestListOrNull to actually match what the method does
  • Checks if there's any object in a collection before trying to parse it (which prevents the "Failed to deserilize object in list" log message)
  • If a date can't be parsed as ISO timestamp, attempts to parse it as millis silently, without printing a log message
  • If op is not defined as part of SpanContext, fallback to an empty string. This field is actually optional in the spec, but we made it non-optional, as we also use the same structure across the project everywhere, where the op field is actually required. So we just duct-tape it for hybrid SDKs here. See https://develop.sentry.dev/sdk/event-payloads/contexts/#trace-context

💡 Motivation and Context

Closes #2151
Closes getsentry/sentry-capacitor#450

💚 How did you test it?

Manually + automated

📝 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 Oct 5, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 371.26 ms 443.66 ms 72.40 ms
Size 1.72 MiB 2.29 MiB 576.40 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4d55d51 389.44 ms 471.55 ms 82.11 ms

App size

Revision Plain With Sentry Diff
4d55d51 1.72 MiB 2.29 MiB 576.34 KiB

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Files Coverage Δ
sentry/src/main/java/io/sentry/JsonSerializer.java 90.67% <100.00%> (ø)
...ry/src/main/java/io/sentry/ProfilingTraceData.java 78.51% <100.00%> (ø)
...entry/src/main/java/io/sentry/SentryBaseEvent.java 90.90% <100.00%> (ø)
sentry/src/main/java/io/sentry/SentryEvent.java 89.43% <100.00%> (ø)
sentry/src/main/java/io/sentry/SpanContext.java 83.43% <100.00%> (+2.71%) ⬆️
...main/java/io/sentry/clientreport/ClientReport.java 62.00% <100.00%> (ø)
...sentry/profilemeasurements/ProfileMeasurement.java 52.72% <100.00%> (ø)
...ry/src/main/java/io/sentry/protocol/DebugMeta.java 90.24% <100.00%> (ø)
...y/src/main/java/io/sentry/protocol/SdkVersion.java 65.34% <100.00%> (ø)
...main/java/io/sentry/protocol/SentryStackTrace.java 94.54% <100.00%> (+0.10%) ⬆️
... and 4 more

📢 Thoughts on this report? Let us know!.

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Nice one, looks good to me!

@romtsn romtsn merged commit 2989106 into main Oct 11, 2023
21 checks passed
@romtsn romtsn deleted the rz/fix/empty-collection-deser branch October 11, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants