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

Remove null keys/values before creating concurrent hashmap #2708

Merged
merged 6 commits into from
May 12, 2023

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented May 11, 2023

📜 Description

Cleanup null values from deserialised extra map before creating a ConcurrentHashMap from it.
ConcurrentHashMap cannot handle null keys or values

💡 Motivation and Context

Fixes #2681

💚 How did you test it?

📝 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

@lbloder
Copy link
Collaborator Author

lbloder commented May 11, 2023

Hey @romtsn and @adinauer,
WDYT, is cleaning up the map before creating the concurrent hash map the right thing to do here? My line of thinking was that it would be nice to keep all valid values in the extras map.
Another possible solution would be to just surround it with try/catch and not adding any extras.

Could the same thing potentially happen with the tags?

@github-actions
Copy link
Contributor

github-actions bot commented May 11, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 334.58 ms 353.80 ms 19.22 ms
Size 1.72 MiB 2.28 MiB 565.47 KiB

Previous results on branch: fix/handle_null_values_event_extras

Startup times

Revision Plain With Sentry Diff
d25f425 304.04 ms 360.30 ms 56.26 ms
cc2c183 285.52 ms 340.71 ms 55.19 ms
5770a02 326.89 ms 361.60 ms 34.71 ms
b5ce73a 299.49 ms 336.00 ms 36.51 ms
f0de9d9 284.78 ms 325.86 ms 41.08 ms

App size

Revision Plain With Sentry Diff
d25f425 1.72 MiB 2.28 MiB 565.47 KiB
cc2c183 1.72 MiB 2.28 MiB 565.47 KiB
5770a02 1.72 MiB 2.28 MiB 565.47 KiB
b5ce73a 1.72 MiB 2.28 MiB 565.54 KiB
f0de9d9 1.72 MiB 2.28 MiB 565.47 KiB

@codecov
Copy link

codecov bot commented May 11, 2023

Codecov Report

Patch coverage: 83.33% and project coverage change: -0.01 ⚠️

Comparison is base (5a573d4) 81.07% compared to head (20013cf) 81.07%.

❗ Current head 20013cf differs from pull request most recent head 81fa32c. Consider uploading reports for the commit 81fa32c to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2708      +/-   ##
============================================
- Coverage     81.07%   81.07%   -0.01%     
- Complexity     4432     4434       +2     
============================================
  Files           345      345              
  Lines         16358    16363       +5     
  Branches       2219     2221       +2     
============================================
+ Hits          13263    13267       +4     
  Misses         2167     2167              
- Partials        928      929       +1     
Impacted Files Coverage Δ
.../src/main/java/io/sentry/util/CollectionUtils.java 57.89% <83.33%> (+3.34%) ⬆️

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

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

Another possible solution would be to just surround it with try/catch and not adding any extras.

I'd prefer keeping as much data as possible.

I think moving the null stripping into CollectionUtils makes sense.

@@ -444,6 +445,15 @@ public boolean deserializeValue(
return true;
case JsonKeys.EXTRA:
Map<String, Object> deserializedExtra = (Map<String, Object>) reader.nextObjectOrNull();
if (deserializedExtra != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we do this in CollectionUtils.newConcurrentHashMap to apply to all places? I presume this problem could happen anywhere we create a ConcurrentHashMap.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think that would make sense.

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

LGTM

@lbloder lbloder marked this pull request as ready for review May 12, 2023 20:30
@lbloder lbloder merged commit a78496a into main May 12, 2023
@lbloder lbloder deleted the fix/handle_null_values_event_extras branch May 12, 2023 20:31
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Remove null keys/values before creating concurrent hashmap ([#2708](https://github.com/getsentry/sentry-java/pull/2708))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 81fa32c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null value in extra map causes event to get silently dropped
2 participants