Skip to content

♻️ [PANA-7375] Eliminate unnecessary conditional node serialization #4629

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.

Sign up for GitHub

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

Open
sethfowler-datadog wants to merge 1 commit into
base: seth.fowler/PANA-7353-clean-up-naming-in-recording-code
Choose a base branch
from
+21 −37
Open

♻️ [PANA-7375] Eliminate unnecessary conditional node serialization #4629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 11 additions & 19 deletions packages/rum-core/src/domain/privacy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,44 +220,36 @@ export function censorText(text: string): string {
return text.replace(/\S/g, TEXT_MASKING_CHAR)
}

export function getTextContent(textNode: Node, parentNodePrivacyLevel: NodePrivacyLevel): string | undefined {
export function getTextContent(textNode: Node, parentNodePrivacyLevel: NodePrivacyLevel): string {
Comment thread
sethfowler-datadog marked this conversation as resolved.
// The parent node may not be a html element which has a tagName attribute.
// So just let it be undefined which is ok in this use case.
const parentTagName = textNode.parentElement?.tagName
let textContent = textNode.textContent || ''

const shouldIgnoreWhiteSpace = parentTagName === 'HEAD'
if (shouldIgnoreWhiteSpace && !textContent.trim()) {
return
}

const nodePrivacyLevel = parentNodePrivacyLevel

const isScript = parentTagName === 'SCRIPT'

if (isScript) {
if (parentTagName === 'SCRIPT') {
// For perf reasons, we don't record script (heuristic)
textContent = CENSORED_STRING_MARK
} else if (nodePrivacyLevel === NodePrivacyLevel.HIDDEN) {
// Should never occur, but just in case, we set to CENSORED_MARK.
textContent = CENSORED_STRING_MARK
} else if (shouldMaskNode(textNode, nodePrivacyLevel)) {
if (
// Scrambling the child list breaks text nodes for DATALIST/SELECT/OPTGROUP
parentTagName === 'DATALIST' ||
parentTagName === 'SELECT' ||
parentTagName === 'OPTGROUP'
) {
if (!textContent.trim()) {
return
}
if (parentTagName === 'DATALIST' || parentTagName === 'SELECT' || parentTagName === 'OPTGROUP') {
// Masking the child list breaks text nodes for DATALIST/SELECT/OPTGROUP.
// TODO: Reinvestigate this.
} else if (parentTagName === 'OPTION') {
// <Option> has low entropy in charset + text length, so use `CENSORED_STRING_MARK` when masked
textContent = CENSORED_STRING_MARK
} else {
textContent = censorText(textContent)
}
}

const shouldCollapseWhiteSpace = parentTagName === 'HEAD'
if (shouldCollapseWhiteSpace && !textContent.trim()) {
return ''
}

return textContent
}

Expand Down
2 changes: 1 addition & 1 deletion packages/rum/src/domain/record/serialization/serializeMutations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ function processCharacterDataMutations(
continue // Mutations to this node should be ignored.
}

const content = getTextContent(node, parentNodePrivacyLevel) ?? ''
const content = getTextContent(node, parentNodePrivacyLevel)
transaction.setText(nodeId, content)
}
}
Expand Down
17 changes: 5 additions & 12 deletions packages/rum/src/domain/record/serialization/serializeNode.node.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,6 @@ describe('serializeNode for DOM nodes', () => {
})
expect(record?.data).toEqual([[ChangeType.AddNode, [null, '#text', '']]])
})

it('does not serialize a whitespace-only node if the parent is a <head> element', async () => {
const record = await serializeHtml('<!doctype HTML><head> </head>', {
input: 'document',
target: (node: Node) => (node as Document).head.firstChild!,
whitespace: 'keep',
})
expect(record?.data).toBeUndefined()
})
})

describe('for HTML elements', () => {
Expand Down Expand Up @@ -184,7 +175,7 @@ describe('serializeNode for DOM nodes', () => {
})

describe('for <head> elements', () => {
it('does not serialize whitespace', async () => {
it('serializes the element', async () => {
const record = await serializeHtml('<!doctype HTML><head> <title> foo </title> </head>', {
input: 'document',
whitespace: 'keep',
Expand All @@ -196,9 +187,11 @@ describe('serializeNode for DOM nodes', () => {
[1, '#doctype', 'html', '', ''],
[0, 'HTML'],
[1, 'HEAD'],
[1, 'TITLE'],
[1, '#text', ''],
[0, 'TITLE'],
[1, '#text', ' foo '],
[4, 'BODY'],
[4, '#text', ''],
[6, 'BODY'],
],
[ChangeType.ScrollPosition, [0, 0, 0]],
])
Expand Down
9 changes: 4 additions & 5 deletions packages/rum/src/domain/record/serialization/serializeNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,8 @@ function serializeTextNode(
privacyLevel: NodePrivacyLevel,
transaction: SerializationTransaction
): void {
const textContent = getTextContent(textNode, privacyLevel)
if (textContent === undefined) {
return
}
const { insertionPoint } = cursor.advance(textNode)
const textContent = getTextContent(textNode, privacyLevel)
transaction.addNode(insertionPoint, '#text', textContent)
}

Expand All @@ -192,7 +189,9 @@ function serializeHiddenNodePlaceholder(
transaction: SerializationTransaction
): void {
// We only generate placeholders for element nodes; other hidden nodes are simply not
// serialized.
// serialized. (But note that a non-element node can only be hidden if it's a descendant
// of another hidden node, since only element nodes can have a different privacy level
// than their parent.)
if (!isElementNode(node)) {
return
}
Expand Down
Loading