search

DataDog / browser-sdk

Datadog Browser SDK
Apache License 2.0
279 stars 130 forks source link

Proposal: Add an option to control whether or not to mask data attribute #2817

Closed chuganzy closed 1 week ago

chuganzy commented 2 weeks ago

Motivation

This PR adds maskDataAttributes option to control whether or not to mask data attributes when privacyLevel is mask.

I see more use of data attributes for styling (CSS), and one of the reasons behind this is because headless accessible React components such as React Aria, Radix and Headless UI are getting more popular, which expose data attributes to enable customizing styles per state.

However, since DataDog masks all data- attributes when privacyPolicy is set to mask, the layout can break in Session Replay.

Currently it is impossible to fix them unless we use lower privacy levels, or stop using data attributes in styles (which is not realistic under the situation)...

Changes

As put in the title, to solve the challenge described above, this PR adds the option to change whether or not to mask data attributes (true by default which enables masking = the current behavior)

Testing

As this is a proposal still, I have not added new tests yet, but I have updated the existing tests already to reflect the changes and yarn test passes locally now!

Please let me know how you feel and I can add some tests as needed!

I have gone over the contributing documentation.

bits-bot commented 2 weeks ago

CLA assistant check
All committers have signed the CLA.

BenoitZugmeyer commented 2 weeks ago

Thank you for your contribution. I wonder if we need to go further and, similarly to other privacy levels, need to support this for parts of the DOM only (ex: <div data-dd-privacy="allow-data-attributes">...</div>). Also, maybe we'll need further customization like maskDataAttributes: (name) => { return name === 'data-my-attribute' }.

We'll discuss with the team and get back to you!

BenoitZugmeyer commented 1 week ago

👋 While we agree this is a valid use-case, we need more reflexion before moving forward on the subject. Unfortunately we don't have the necessary bandwidth to work on this for now. In the meantime, feel free to use your Browser SDK fork on your project!

chuganzy commented 1 week ago

@BenoitZugmeyer Got it, thank you for sharing. It is unfortunate to hear that but looking forward to the future updates!

Just to share what I have considered before adding this field, there is an allowed list already exists there and I thought adding some advanced options such as (name: string) => name === 'something' makes it inconsistent.. Also adding some sort of pattern (regex) filter (as well as a heavy filter function) might affect the performance..

https://github.com/DataDog/browser-sdk/blob/1792105abbd1b45e62968afee2ab9e7e63c45530/packages/rum-core/src/domain/getSelectorFromElement.ts#L9

I have thought of adding a new privacy level too, however it might end up with adding many levels to control other fields or conflicting with other levels..

I thought maybe we can add a very simple option to just toggle it as the first step, and later on when these challenges are solved, can add these advanced filter options separately, or as union types, but this is jut my two cents!