Skip to content

Drive marker PII sanitization from schemas - #6291

Merged
fatadel merged 2 commits into
firefox-devtools:mainfrom
fatadel:issue-6290
Sep 11, 2026
Merged

Drive marker PII sanitization from schemas#6291
fatadel merged 2 commits into
firefox-devtools:mainfrom
fatadel:issue-6290

Conversation

@fatadel

@fatadel fatadel commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Main | Deploy preview

Markers requiring PII sanitization were identified through hardcoded data.type checks. Putting PII directives in marker schema fields removes type-specific branching and makes sanitization generic.

Closes #6290


Profile

Published using Prod | Published using Deploy Preview

@fatadel
fatadel requested review from canova and mstange August 31, 2026 13:09
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.45455% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.88%. Comparing base (18e46d8) to head (b5c6842).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/profile-logic/marker-data.ts 92.42% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6291      +/-   ##
==========================================
+ Coverage   83.84%   83.88%   +0.04%     
==========================================
  Files         352      353       +1     
  Lines       37807    37905      +98     
  Branches    10674    10683       +9     
==========================================
+ Hits        31698    31797      +99     
  Misses       5679     5679              
+ Partials      430      429       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@canova
canova requested review from canova and removed request for canova and mstange September 3, 2026 14:35

@canova canova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I have some high level schema related question, let me know what you think!

Comment thread src/types/markers.ts Outdated
Comment thread src/types/markers.ts Outdated
Comment thread src/types/markers.ts Outdated
@mstange

mstange commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

I haven't looked at the code yet but I'd like to point out #5301 which has a related proposal.

@canova

canova commented Sep 9, 2026

Copy link
Copy Markdown
Member

@mstange ah thanks! I forgot that this was filed. Structurally it's actually very similar to what I suggested (minus the naming differences) but I forgot the sanitized-string one. It looks nice

@fatadel

fatadel commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@mstange ah thanks! I forgot that this was filed. Structurally it's actually very similar to what I suggested (minus the naming differences) but I forgot the sanitized-string one. It looks nice

Let's handle sanitized-string in that issue then and keep this one's scope to the one we already defined?

@fatadel
fatadel requested a review from canova September 9, 2026 12:01
@fatadel

fatadel commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @canova! I've addressed the issues now.

Comment thread src/types/markers.ts Outdated
@fatadel

fatadel commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, @mstange! Done now ✅

@mstange

mstange commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

I don't see an upgrader; what happens if you re-upload an existing old uploaded profile and uncheck some checkboxes in the re-upload?

@mstange

mstange commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Also does there need to be code in profile processing which adds these annotations when converting from a Gecko profile?

@fatadel

fatadel commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

@mstange Both concerns are valid. My bad. Fixed now.

@canova canova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me!

Comment thread src/profile-logic/marker-data.ts Outdated
Comment thread src/profile-logic/marker-data.ts
Markers requiring PII sanitization were identified through hardcoded
`data.type` checks. Describing PII categories in marker schema fields
removes type-specific branching while leaving sanitization behavior to
the consumer.
@fatadel
fatadel merged commit a7ff1a5 into firefox-devtools:main Sep 11, 2026
23 checks passed
@fatadel
fatadel deleted the issue-6290 branch September 11, 2026 13:22
@canova

canova commented Sep 11, 2026

Copy link
Copy Markdown
Member

Re extension id removal code: after thinking about it again, I think we should have converted these extension related markers to real marker schema types with their own payload and convert them in the upgrader + the processing phase. This would allows us remove the markerName need, and we could fix the gecko side later and have a gecko profile format bump later.

Sorry for not catching it sooner!

@fatadel

fatadel commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Re extension id removal code: after thinking about it again, I think we should have converted these extension related markers to real marker schema types with their own payload and convert them in the upgrader + the processing phase. This would allows us remove the markerName need, and we could fix the gecko side later and have a gecko profile format bump later.

Sorry for not catching it sooner!

I'll create a follow-up PR before we deploy, no worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Drive marker PII sanitization from marker schemas

3 participants