Skip to content

RUM-2459: Safe serialization of user-provided attributes#1818

Merged
0xnm merged 2 commits into
developfrom
nogorodnikov/rum-2459/safe-serialization-of-user-provided-properties
Jan 12, 2024
Merged

RUM-2459: Safe serialization of user-provided attributes#1818
0xnm merged 2 commits into
developfrom
nogorodnikov/rum-2459/safe-serialization-of-user-provided-properties

Conversation

@0xnm

@0xnm 0xnm commented Jan 11, 2024

Copy link
Copy Markdown
Member

What does this PR do?

This PR brings safer serialization of user-provided attributes, so that any exception thrown during this process won't discard the whole event (span, log, RUM).

It consists of 2 improvements:

  • Serialization of additionalProperties (typically exists in the usr data and in the top-level event context) won't break the serialization of the parent event. Instead, if some particular item failed serialization, it will be dropped and serialization will continue. Both key and value will be dropped, this is a consistent behavior with drop of the reserved/non-conformant attributes.
  • We will make a copy of user-provided attributes in RumMonitor at the call site. This is to prevent ConcurrentModificationException which sometimes happen when we iterate over them later. Making a copy at the call-site has a benefit of doing it on the user-thread, which lowers the chance that concurrent modification is done at the same time, and if there is such case, exception will be thrown on the user thread and will be more visible.

Both improvements operate only on the top-level items and don't go deep, this is enough to bring the necessary safety.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@0xnm
0xnm requested review from a team as code owners January 11, 2024 16:21
@0xnm
0xnm force-pushed the nogorodnikov/rum-2459/safe-serialization-of-user-provided-properties branch 2 times, most recently from 16ba574 to 2c56536 Compare January 11, 2024 16:52
@codecov-commenter

codecov-commenter commented Jan 11, 2024

Copy link
Copy Markdown

Codecov Report

Merging #1818 (ef60e47) into develop (04d4a93) will increase coverage by 0.01%.
Report is 1 commits behind head on develop.
The diff coverage is 98.33%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1818      +/-   ##
===========================================
+ Coverage    83.49%   83.49%   +0.01%     
===========================================
  Files          467      467              
  Lines        16412    16448      +36     
  Branches      2482     2482              
===========================================
+ Hits         13702    13733      +31     
- Misses        2032     2037       +5     
  Partials       678      678              
Files Coverage Δ
...id/log/internal/domain/event/LogEventSerializer.kt 100.00% <100.00%> (ø)
...id/rum/internal/domain/event/RumEventSerializer.kt 100.00% <100.00%> (ø)
.../android/rum/internal/monitor/DatadogRumMonitor.kt 84.43% <100.00%> (+0.33%) ⬆️
...trace/internal/domain/event/SpanEventSerializer.kt 93.02% <92.86%> (-0.31%) ⬇️

... and 25 files with indirect coverage changes

@fuzzybinary

Copy link
Copy Markdown
Member

I may want to pull similar functionality into the Flutter SDK, specifically for calling the Mapper. Is the safeMapValuesToJson available to me or will I have to copy it?

@0xnm

0xnm commented Jan 12, 2024

Copy link
Copy Markdown
Member Author

@fuzzybinary safeMapValuesToJson is public-internal API, so you can use it.

@0xnm
0xnm force-pushed the nogorodnikov/rum-2459/safe-serialization-of-user-provided-properties branch from 2c56536 to 5a32640 Compare January 12, 2024 08:08

@mariusc83 mariusc83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great work

@0xnm
0xnm force-pushed the nogorodnikov/rum-2459/safe-serialization-of-user-provided-properties branch from 5a32640 to ef60e47 Compare January 12, 2024 08:29
@0xnm
0xnm merged commit b8dc955 into develop Jan 12, 2024
@0xnm
0xnm deleted the nogorodnikov/rum-2459/safe-serialization-of-user-provided-properties branch January 12, 2024 08:53
@xgouchet xgouchet added this to the 2.5.0 milestone Feb 19, 2024
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.

5 participants