-
Notifications
You must be signed in to change notification settings - Fork 157
✨ [RUM-10415] Add support for action name allowlist masking #3648
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3648 +/- ##
==========================================
- Coverage 92.44% 92.36% -0.08%
==========================================
Files 322 324 +2
Lines 8129 8215 +86
Branches 1838 1862 +24
==========================================
+ Hits 7515 7588 +73
- Misses 614 627 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8e29d87
to
dc2ecea
Compare
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
9654e39
to
489bf17
Compare
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.spec.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
lifeCycle.notify(LifeCycleEventType.RAW_RUM_EVENT_COLLECTED, processAction(action)) | ||
) | ||
const actionNameDictionary = createActionAllowList() | ||
addAllowlistObserver(actionNameDictionary) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could createActionAllowList()
register the observer? It feels like this code shouldn't need to know about that. (Maybe createActionAllowList()
is really startActionAllowListObserver()
or something?)
Going a bit further: right now, there is no code that removes the allowlist observer when startActionCollection()
's stop()
is called. So we create a new dictionary and add another observer whenever startActionCollection()
is called, but nothing ever removes them, and they keep building up. I don't think we want that.
We should pick one of two approaches:
- Use a single global allowlist observer and dictionary, and never replace them or clear them. (Reasonable, since
$DD_ALLOW
is also global.) - Register the allowlist observer and set up the dictionary when action collection starts; unregister the allowlist observer and clear the underlying dictionary when action collection stops.
The first avoids reprocessing the raw strings when recording restarts, so it has a performance benefit, but naturally you'll have to add some additional affordances for testing with that approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
With the first approach, do we need end-to-end tests or just keeping the contexts in unit tests would be enough? I'm ok to proceed with the second approach for now but clearing the dictionary and re-process could get expensive. We might need some field data on this.
Fixed with the 2nd approach.
|
||
let masked = false | ||
return { | ||
name: name.replace(getMatchRegex(), (word: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ question: IIUC, when multiple tokens are masked, we might end up with a name like "MASKED MASKED MASKED", which could be confusing from a UI perspective. Especially, given that the other masking strategy displays "Masked Element", shouldn’t we aim for consistency? Do we have product input on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, although Masked Element
is not idea for tokenized the names either, maybe we should seek another way, ie XXX XXX in session replay. Will ping product on this.
@@ -14,8 +14,9 @@ export const enum ActionNameSource { | |||
TEXT_CONTENT = 'text_content', | |||
STANDARD_ATTRIBUTE = 'standard_attribute', | |||
BLANK = 'blank', | |||
MASK_DISALLOWED = 'mask_disallowed', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: I find this source name confusing. As mentioned in my other comment, I think we should ensure consistent behavior across our masking strategies. That way, we can keep using MASK_PLACEHOLDER as the source. If we need to identify which masking strategy was applied, we could include it as a separate event attribute. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, yes we would like to distinguish the two strategies. We did add mask_disallowed
as a name source in rum-event-format
as a separate attribute. But I'm open to change it to another name.
1de7145
to
ce564c5
Compare
617489d
to
129b1aa
Compare
129b1aa
to
549159f
Compare
16affe6
to
6f20b2b
Compare
d69e70c
to
6e5063b
Compare
7c8f655
to
5105112
Compare
/to-staging |
View all feedbacks in Devflow UI.
Commit 5105112a2d will soon be integrated into staging-28.
Commit 5105112a2d has been merged into staging-28 in merge commit e142f86173. Check out the triggered pipeline on Gitlab 🦊 If you need to revert this integration, you can use the following command: |
…o staging-28 Integrated commit sha: 5105112 Co-authored-by: cy-moi <[email protected]>
packages/rum-core/src/domain/action/privacy/allowedDictionary.spec.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.spec.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
const dictionary: AllowedDictionary = { | ||
rawStringCounter: 0, | ||
allowlist: new Set<string>(), | ||
rawStringIterator: window.$DD_ALLOW?.values(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you may as well just let processRawAllowList()
handle initializing this, and set it to undefined
here. That way, it always gets initialized using the same mechanism.
packages/rum-core/src/domain/action/privacy/allowedDictionary.ts
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
const { splitRegex } = cachedRegexes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: perhaps, to avoid the need for a non-null assertion here, you could replace initializeUnicodeSupport()
with a function that returns the regular expressions if Unicode regular expressions are supported, and returns undefined
otherwise? So you'd have e.g.:
const regexes = getOrInitRegexes();
if (!regexes) {
// No Unicode regular expression support; fall back.
return
}
// Unicode regular expressions were supported; use them!
const maskedName = name.replace(regexes.split, // ...
aea0f2c
to
da92015
Compare
ca8df7f
to
045d430
Compare
Motivation
Mask action name with allowlists generated from rum privacy build plugin(WIP). This approach is purely client side and allowlist-based. We aim to mask all action names (custom & auto) OOTB with build time configuration using a build plugin.
The raw string literals would be extracted at build time and loaded on demand in runtime with pre-injected helpers. In SDK, we only need logics of tokenizing raw strings and saving to a dictionary, which will used to mask action names (tokenized in the same way).
Changes
Test instructions
Tests with BrowserStack should pass (for regEx compatibility)
Checklist