✨ [RUM-162] Truncate resources URL containing data URLs #2690

cy-moi · 2024-04-05T11:52:14Z

Motivation

We're currently collecting the full url data of resources using data: based resource.url .
This is highly inefficient for the ingestion and indexing side and may contain sensitive informations. This data is always truncated to 24k characters when being indexed, which would result in partial images.
In this case, we want to truncate the partial data till only keep the useful information (MIME type, encodings).

Changes

We truncate data:url to the embedding codec when they exceeds 24k
Examples

Edit (Added truncate indicator [...])

Testing

Local
Staging
Unit
End to end

I have gone over the contributing documentation.

cit-pr-commenter · 2024-04-05T11:57:43Z

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	156.48 KiB	156.64 KiB	160 B	+0.10%	✅
Logs	55.34 KiB	55.34 KiB	0 B	0.00%	✅
Rum Slim	102.98 KiB	103.18 KiB	208 B	+0.20%	✅
Worker	25.21 KiB	25.21 KiB	0 B	0.00%	✅

codecov-commenter · 2024-04-05T11:58:18Z

Codecov Report

Attention: Patch coverage is 92.85714% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 93.36%. Comparing base (61258c1) to head (3f1f4c4).
Report is 10 commits behind head on main.

Files	Patch %	Lines
...ages/rum-core/src/domain/resource/resourceUtils.ts	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2690      +/-   ##
==========================================
- Coverage   93.36%   93.36%   -0.01%     
==========================================
  Files         240      240              
  Lines        6981     6992      +11     
  Branches     1539     1542       +3     
==========================================
+ Hits         6518     6528      +10     
- Misses        463      464       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cy-moi · 2024-04-05T12:08:07Z

/to-staging

dd-devflow · 2024-04-05T12:08:13Z

🚂 Branch Integration: starting soon, merge in < 10m

Commit 477629a771 will soon be integrated into staging-14.

This build is going to start soon! (estimated merge in less than 10m)

Use /to-staging -c to cancel this operation!

…resources into staging-14 Co-authored-by: cy-moi <[email protected]>

dd-devflow · 2024-04-05T12:17:35Z

🚂 Branch Integration: This commit was successfully integrated

Commit 477629a771 has been merged into staging-14 in merge commit 8ca824d45b.

Check out the triggered pipeline on Gitlab 🦊

amortemousque · 2024-04-05T13:39:54Z

packages/rum/src/domain/record/serialization/serializeAttribute.ts

-
-  return attributeValue
+  // Truncate data:url to avoid performance impact
+  return findDataUrlAndTruncate(attributeValue) ?? attributeValue


❓ question: ‏You are truncating all data:url not only the one above a MAX_ATTRIBUTE_VALUE_CHAR_LENGTH. Is it intended? If yes, could it have an impact on Replay?

Yes, it was intended. But then indeed the truncation would prevent some allowed data to show up properly. So fixing on this right now.

I've added the length check back to the recorder as well as for the resource url.

packages/rum-core/src/domain/resource/resourceUtils.ts

packages/rum-core/src/domain/resource/resourceUtils.spec.ts

packages/rum-core/src/domain/resource/resourceUtils.ts

packages/rum-core/src/domain/resource/resourceCollection.ts

cy-moi · 2024-04-08T14:35:24Z

/to-staging

dd-devflow · 2024-04-08T14:35:33Z

🚂 Branch Integration: starting soon, merge in < 0s

Commit 9b3336f9e3 will soon be integrated into staging-15.

This build is going to start soon! (estimated merge in less than 0s)

Use /to-staging -c to cancel this operation!

dd-devflow · 2024-04-08T14:37:46Z

🚨 Branch Integration: The build pipeline contains failing jobs for this merge request

We couldn't automatically merge the commit 9b3336f9e3 into staging-15.
Build pipeline has failing jobs for 151b347

Since those jobs are not marked as being allowed to fail, the pipeline will most likely fail.
Therefore, and to allow other builds to be processed, this merge request has been rejected before the end of the pipeline.

You should have a look at the pipeline, wait for the build to finish and investigate the failures.
When you are ready, re-add your pull request to the queue!

⚠️

Do NOT retry failed jobs directly.

They were executed on a temporary branch created by the merge queue. If you retry them, they are going to fail because the branch no longer exists.

If you think the errors come from a logical conflict with the target branch, you can create a fix by commenting this pull request with /create-fix-branch -b staging-15

Details

List of failed jobs:

check-staging-merge

Go to failed Gitlab pipeline.

If you need support, contact us on Slack #devflow with those details!

cy-moi · 2024-04-08T14:45:29Z

/create-fix-branch -b staging-15

dd-devflow · 2024-04-08T14:45:36Z

🚂 Devflow: /create-fix-branch -b staging-15

Created fix branch fix-merge-9b3336f9e3-into-staging-15 - #2698

dd-devflow · 2024-04-08T14:50:09Z

🚂 Branch Integration: starting soon, merge in < 0s

Commit 9b3336f9e3 will soon be integrated into staging-15.

This build is going to start soon! (estimated merge in less than 0s)

dd-devflow · 2024-04-08T14:59:33Z

🚂 Branch Integration

Commit 9b3336f9e3 has been merged into staging-15 in merge commit 2f8877703d.

Check out the triggered pipeline on Gitlab 🦊

thomas-lebeau · 2024-04-09T06:57:32Z

packages/rum-core/src/domain/resource/resourceUtils.ts

+const DATA_URL_REGEX = /data:(.+)?(;base64)?,/g
+export const MAX_ATTRIBUTE_VALUE_CHAR_LENGTH = 24_000
+
+export function isDataUrlTooLong(url: string): boolean {


💬 suggestion: ‏ this function name suggests it's taking a data URL as an input and check if it's too long, while what it is actually doing is taking a string as en input check if it's a data URL and if it's long.

I propose to either:

rename to something like isLongDataUrl (I think the too is not adding much value)

split the function in two such as isAboveLimit and isDataUrl

Thanks! I searched the codebase and it seems like we are only using this limit length for data URL, hence making it into separate functions feels redundant. But the naming is indeed not accurate. I have renamed it to isLongDataUrl.

packages/rum-core/src/domain/resource/resourceUtils.spec.ts

amortemousque · 2024-04-10T11:06:09Z

packages/rum-core/src/domain/resource/resourceCollection.ts

Shouldn't we also truncate the url coming from performance resource entries?

browser-sdk/packages/rum-core/src/domain/resource/resourceCollection.ts

Line 146 in 160d5fe

url: entry.name,

Notes on our slack discussion, we found that here it says:

If an HTML IMG element has a data: URI as its source [RFC2397], then this resource will not be included as a PerformanceResourceTiming object in the Performance Timeline. By definition data: URI contains embedded data and does not require a fetch.

So we do not sanitize data url in this case.

amortemousque · 2024-04-10T14:04:26Z

packages/rum-core/src/domain/resource/resourceUtils.ts

+}
+
+export function sanitizeDataUrl(url: string): string {
+  return url.match(DATA_URL_REGEX)![0]


💬 suggestion:
It could be nice to have something indicating that it the url has been truncated like data:[<mediatype>] [...] (the same way we do for action names)

I've added the [...] as in action names to indicate the truncation.

cy-moi added 3 commits April 4, 2024 16:46

Add truncate data:url util function and tests

a062959

Move to rum-core and reuse in recorder

f46a202

Fix import export

477629a

dd-mergequeue bot added a commit that referenced this pull request Apr 5, 2024

Integrate 477629a (#2690) from congyao/RUM-162-truncate-data-url-for-…

8ca824d

…resources into staging-14 Co-authored-by: cy-moi <[email protected]>

dd-devflow bot added the staging-14 label Apr 5, 2024

clean unit test for serialization

1a6976f

amortemousque reviewed Apr 5, 2024

View reviewed changes

cy-moi added 4 commits April 8, 2024 11:47

Add checking for max length 24k

20c1edc

Remove unused import

bcf0d5d

Simplify regex

d3bb4d4

Add length check for resource url

9b3336f

cy-moi requested a review from amortemousque April 8, 2024 14:34

cy-moi changed the title ~~[RUM-162] Truncate resources URL containing data URLs~~ ✨ [RUM-162] Truncate resources URL containing data URLs Apr 8, 2024

dd-devflow bot added the staging-15 label Apr 8, 2024

cy-moi marked this pull request as ready for review April 8, 2024 16:23

cy-moi requested a review from a team as a code owner April 8, 2024 16:23

thomas-lebeau reviewed Apr 9, 2024

View reviewed changes

Rename function; Fix test description

160d5fe

cy-moi requested a review from thomas-lebeau April 9, 2024 10:54

N-Boutaib approved these changes Apr 9, 2024

View reviewed changes

thomas-lebeau approved these changes Apr 9, 2024

View reviewed changes

amortemousque reviewed Apr 10, 2024

View reviewed changes

cy-moi added 3 commits April 10, 2024 17:16

add [...] to indicate truncate

ad07338

🐛 Fix string match error when the url is tooo long

9a88d41

Clean code

3f1f4c4

amortemousque approved these changes Apr 11, 2024

View reviewed changes

cy-moi merged commit 2045899 into main Apr 11, 2024
17 checks passed

cy-moi deleted the congyao/RUM-162-truncate-data-url-for-resources branch April 11, 2024 14:33

cy-moi restored the congyao/RUM-162-truncate-data-url-for-resources branch April 11, 2024 16:58

cy-moi deleted the congyao/RUM-162-truncate-data-url-for-resources branch April 12, 2024 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ [RUM-162] Truncate resources URL containing data URLs #2690

✨ [RUM-162] Truncate resources URL containing data URLs #2690

cy-moi commented Apr 5, 2024 •

edited

Loading

cit-pr-commenter bot commented Apr 5, 2024 •

edited

Loading

codecov-commenter commented Apr 5, 2024 •

edited

Loading

cy-moi commented Apr 5, 2024

dd-devflow bot commented Apr 5, 2024

dd-devflow bot commented Apr 5, 2024

amortemousque Apr 5, 2024

cy-moi Apr 5, 2024 •

edited

Loading

cy-moi Apr 8, 2024

cy-moi commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

cy-moi commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

thomas-lebeau Apr 9, 2024

cy-moi Apr 9, 2024

amortemousque Apr 10, 2024

cy-moi Apr 11, 2024

amortemousque Apr 10, 2024

cy-moi Apr 10, 2024

✨ [RUM-162] Truncate resources URL containing data URLs #2690

✨ [RUM-162] Truncate resources URL containing data URLs #2690

Conversation

cy-moi commented Apr 5, 2024 • edited Loading

Motivation

Changes

Testing

cit-pr-commenter bot commented Apr 5, 2024 • edited Loading

Bundles Sizes Evolution

codecov-commenter commented Apr 5, 2024 • edited Loading

Codecov Report

cy-moi commented Apr 5, 2024

dd-devflow bot commented Apr 5, 2024

dd-devflow bot commented Apr 5, 2024

Choose a reason for hiding this comment

cy-moi Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cy-moi commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

cy-moi commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

dd-devflow bot commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cy-moi commented Apr 5, 2024 •

edited

Loading

cit-pr-commenter bot commented Apr 5, 2024 •

edited

Loading

codecov-commenter commented Apr 5, 2024 •

edited

Loading

cy-moi Apr 5, 2024 •

edited

Loading