Skip to content

[RUM-13793] 🐛 Skip potential sanitize updates on unaltered fields#4298

Merged
bdibon merged 12 commits into
mainfrom
boris.dibon/fix_rum-13793
Mar 12, 2026
Merged

[RUM-13793] 🐛 Skip potential sanitize updates on unaltered fields#4298
bdibon merged 12 commits into
mainfrom
boris.dibon/fix_rum-13793

Conversation

@bdibon
Copy link
Copy Markdown
Contributor

@bdibon bdibon commented Mar 9, 2026

Motivation

When a RumResourceEvent has a resource.url field that is too long one of two things can happen:

  1. beforeSend is used: the sanitize() function will return undefined and the limitModification function will set the resource.url field to undefined, effectively deleting it. This caused problem on web-ui (now fixed).

  2. beforeSend is not used: the createBatch() function will discard the message that contains the RumResourceEvent, we're loosing data.


Reflection behind the changes

What do we even fix?

Sending invalid data

The problem that surfaced to our users is now fixed, web-ui doesn't break when a RumResourceEvent has no resource.url.

The SDK shouldn't send invalid data to intake, this can be fixed in several ways:

  • For the specific resource.url field we can simply truncate it instead of setting it to undefined, then "how much do we truncate?", at lest enough for the message to intake to not be discarded
  • Have a generic fix that replaces every field sanitized to undefined by either "" or {}

Inconsistent behavior

Also note the SDK is behaving in an inconsistent manner:

  • if you don't pass a beforeSend() in the RumInitConfiguration, events with unsafe fields (large URL for example) will be discarded by the transport layer. The event is discarded
  • if you do pass a beforeSend() that doesn't alter the event, unsafe fields will be sanitized, which mean they will be either set to undefined (string too long), or truncated (the object is too big); The event or context is updated while the user never intended to do it.

Summary

Then we have two problems to solve:

  1. invalid data sent to intake
  2. inconsistent behavior with beforeSend(),

We solve 1. by not erasing required fields.

We solve 2. by not applying the sanitize process to fields the user hasn't modified, this requires us to compare the original value object[field] with the new value in setNestedValue(). This is easy for string values, but the implementation might be more involved for objects and potentially slow.

For this PR, I've have decided to fix 1 completely and 2 partially.

For string values, we don't go through the sanitize process if they didn't change.

For object values, we still go through it, at the risk of tempering the value if it's a large object (serialization wise), meaning the resulting object might be truncated. I think this is an acceptable outcome given the low probability of occurence (objects whose serialized version length is superior to 225 280 characters).

Changes

After second feedback round:

  • Skip sanitize() calls for untouched string values
  • Add a related unit test
Changes history

After first feedback round:

  • When a field is erased by the sanitize function, just replace it with "" or {} depending on the fieldType
  • Add a related unit test

Before feedback:

  • Save resource.url before beforeSend runs, so the original value can be restored if the user callback corrupts it
  • Truncate resource.url to 32 KiB if it exceeds that limit (or was set to undefined by beforeSend), with a display.warn message
  • Add two unit tests in assembly.spec.ts covering truncation both with and without a beforeSend callback.

Test instructions

  • Manual test (use the sandbox):
<script>
  fetch('/' + 'a'.repeat(300_000))
</script>
  • Unit tests:
yarn test:unit --spec 'packages/rum-core/src/domain/limitModification.spec.ts'

Checklist

  • Tested locally
  • Tested on staging
  • Added unit tests for this change.
  • Added e2e/integration tests for this change.
  • Updated documentation and/or relevant AGENTS.md file

@bdibon bdibon requested a review from a team as a code owner March 9, 2026 13:51
@bdibon bdibon marked this pull request as draft March 9, 2026 13:52
@bdibon bdibon force-pushed the boris.dibon/fix_rum-13793 branch from 0a59001 to c9c4d23 Compare March 9, 2026 13:54
@cit-pr-commenter-54b7da
Copy link
Copy Markdown

cit-pr-commenter-54b7da Bot commented Mar 9, 2026

Bundles Sizes Evolution

📦 Bundle Name Base Size Local Size 𝚫 𝚫% Status
Rum 174.53 KiB 174.54 KiB +19 B +0.01%
Rum Profiler 6.16 KiB 6.16 KiB 0 B 0.00%
Rum Recorder 27.46 KiB 27.46 KiB 0 B 0.00%
Logs 56.84 KiB 56.84 KiB 0 B 0.00%
Flagging 944 B 944 B 0 B 0.00%
Rum Slim 130.21 KiB 130.23 KiB +19 B +0.01%
Worker 23.63 KiB 23.63 KiB 0 B 0.00%
🚀 CPU Performance
Action Name Base CPU Time (ms) Local CPU Time (ms) 𝚫%
RUM - add global context 0.0047 0.0058 +23.40%
RUM - add action 0.0183 0.0163 -10.93%
RUM - add error 0.0147 0.0171 +16.33%
RUM - add timing 0.0032 0.0034 +6.25%
RUM - start view 0.0134 0.0162 +20.90%
RUM - start/stop session replay recording 0.0008 0.0009 +12.50%
Logs - log message 0.0197 0.0182 -7.61%
🧠 Memory Performance
Action Name Base Memory Consumption Local Memory Consumption 𝚫
RUM - add global context 26.88 KiB 27.00 KiB +122 B
RUM - add action 51.21 KiB 52.47 KiB +1.26 KiB
RUM - add timing 26.72 KiB 27.53 KiB +828 B
RUM - add error 56.25 KiB 53.87 KiB -2.38 KiB
RUM - start/stop session replay recording 25.88 KiB 26.30 KiB +430 B
RUM - start view 452.02 KiB 448.74 KiB -3.28 KiB
Logs - log message 45.00 KiB 44.44 KiB -566 B

🔗 RealWorld

@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented Mar 9, 2026

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage (details)
Patch Coverage: 66.67%
Overall Coverage: 77.19% (-0.01%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 5fa77a1 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback!

Comment thread packages/rum-core/src/domain/assembly.ts Outdated
Comment thread packages/rum-core/src/domain/assembly.ts Outdated
Comment thread packages/rum-core/src/domain/assembly.ts Outdated
@bdibon bdibon force-pushed the boris.dibon/fix_rum-13793 branch 3 times, most recently from 5f2356e to 18017a5 Compare March 11, 2026 12:07
Comment thread packages/rum-core/src/domain/limitModification.ts Outdated
Comment thread packages/rum-core/src/domain/limitModification.ts Outdated
Comment thread packages/rum-core/src/domain/limitModification.spec.ts Outdated
@bdibon bdibon changed the title [RUM-13793] 🐛 Double check illegal payloads from resource events (URL too long) [RUM-13793] 🐛 Skip potential sanitize udates on unaltered fields Mar 11, 2026
@bdibon bdibon changed the title [RUM-13793] 🐛 Skip potential sanitize udates on unaltered fields [RUM-13793] 🐛 Skip potential sanitize updates on unaltered fields Mar 11, 2026
@bdibon bdibon force-pushed the boris.dibon/fix_rum-13793 branch from f7ab50d to e59859e Compare March 12, 2026 08:48
@bdibon bdibon requested a review from bcaudan March 12, 2026 08:49
@bdibon bdibon marked this pull request as ready for review March 12, 2026 09:39
Comment thread packages/rum-core/src/domain/limitModification.spec.ts Outdated
bdibon and others added 12 commits March 12, 2026 14:40
Revert the following commits:
- c9c4d23 🐛 Double check illegal payloads from resource events (URL too long)
- 60ba402 ✅ Test beforeSend doesn't erase resource.url field
- e920d90 🐛 Add a check to prevent illegal updates to resource events

Made-with: Cursor
With the current limitModification implementation, if the modifier doesn't perform any update on serverRumEvent.resource.url and the url happens to be too long, then it will be reset to undefined.

This is an "undefined behavior" that creates a discrepancy when the user of the SDK doesn't provide a beforeSend in the init config.

This commit solves the discrepancy by not calling sanitize on fields whose value (is a string) and hasn't been altered by the modifier.
Co-authored-by: Bastien Caudan <1331991+bcaudan@users.noreply.github.com>
@bdibon bdibon force-pushed the boris.dibon/fix_rum-13793 branch from e59859e to 5fa77a1 Compare March 12, 2026 13:40
@bdibon
Copy link
Copy Markdown
Contributor Author

bdibon commented Mar 12, 2026

/to-staging

@gh-worker-devflow-routing-ef8351
Copy link
Copy Markdown

gh-worker-devflow-routing-ef8351 Bot commented Mar 12, 2026

View all feedbacks in Devflow UI.

2026-03-12 13:41:02 UTC ℹ️ Start processing command /to-staging


2026-03-12 13:41:08 UTC ℹ️ Branch Integration: starting soon, merge expected in approximately 17m (p90)

Commit 5fa77a13ce will soon be integrated into staging-11.


2026-03-12 13:56:24 UTC ℹ️ Branch Integration: this commit was successfully integrated

Commit 5fa77a13ce has been merged into staging-11 in merge commit b57398e161.

Check out the triggered DDCI request.

If you need to revert this integration, you can use the following command: /code revert-integration -b staging-11

gh-worker-dd-mergequeue-cf854d Bot added a commit that referenced this pull request Mar 12, 2026
Integrated commit sha: 5fa77a1

Co-authored-by: bdibon <boris.dibon@datadoghq.com>
@bdibon bdibon merged commit 93fe0a2 into main Mar 12, 2026
22 checks passed
@bdibon bdibon deleted the boris.dibon/fix_rum-13793 branch March 12, 2026 15:35
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 12, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants