Skip to content
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

report: group third-party entities #14655

Merged
merged 55 commits into from
Mar 6, 2023
Merged

report: group third-party entities #14655

merged 55 commits into from
Mar 6, 2023

Conversation

alexnj
Copy link
Member

@alexnj alexnj commented Jan 9, 2023

Part 3, "entity grouping and aggregation" changes of the design doc. This introduces grouping of audit results by entities, with aggregated values where applicable.

Summary of changes:

  • All audits that deal with URLs are grouped by entity.
  • Introduces a heading/group row to tables that feature: Entity's name, a category chip (or "1st Party") indicating the category of 3P, and a link to open the homepage of 3P (on hover).
  • Report-side aggregate calculations. Columns with units bytes, numeric, ms, timespanMs are totaled by default. Can be avoided by dontAggregate: boolean flag on a column.
  • Allow pre-grouped audit results (covers ThirdPartySummary audit).
  • Introduce a sortBy directive in Table and Opportunity that indicates to report how aggregation logic should sort rows.

Pending fixes:

  • Update base branch to main once report: use entity classification to filter third-parties #14697 lands.
  • Fix issue with double-counting third-party group rows in report filter.
  • Fix missing category names.
  • i18n of strings.
  • Update/add tests.
    • Grouping
    • Category name chips groups
    • 1st party chip in groups
    • Legacy non-grouped report and filtering (ensure it's working)
  • Convert ThirdPartySummary.entity from a LinkValue to a string entity name.

Screenshot of Unused Javascript audit report below:

image

@alexnj alexnj requested a review from a team as a code owner January 9, 2023 22:00
@alexnj alexnj requested review from adamraine and removed request for a team January 9, 2023 22:00
@alexnj alexnj force-pushed the entity-based-3p-grouping branch from b5ddaed to 715a728 Compare January 18, 2023 22:29
@alexnj alexnj changed the base branch from entity-based-3p to entity-based-3p-reportonly January 18, 2023 22:33
@alexnj alexnj added the 10.0 label Jan 23, 2023
Base automatically changed from entity-based-3p-reportonly to main January 25, 2023 00:16
@alexnj alexnj changed the base branch from main to entity-based-3p-reportonly-extn January 27, 2023 01:13
@alexnj alexnj force-pushed the entity-based-3p-grouping branch from 715a728 to 5b5bc80 Compare January 27, 2023 01:17
@alexnj alexnj changed the title [WIP] report: group third-party entities wip: report: group third-party entities Jan 27, 2023
Base automatically changed from entity-based-3p-reportonly-extn to main January 27, 2023 22:23
@connorjclark
Copy link
Collaborator

connorjclark commented Jan 28, 2023

Can you address the "159 commits" thing? Maybe you need to squash+rebase with origin/main?

@alexnj
Copy link
Member Author

alexnj commented Jan 28, 2023

Maybe you need to squash

Done.

@connorjclark connorjclark added 10.1 and removed 10.0 labels Feb 3, 2023
@adamraine
Copy link
Member

There are changes to the lhr like sortBy, skipSumming etc. I think it would be good to land those changes in 10.0 so that the 10.1 report renderer can handle 10.0 lhrs.

@connorjclark
Copy link
Collaborator

connorjclark commented Feb 3, 2023

Those are additions, not breaking changes. 10.1 renderer (assuming with this PR) would just render 10.0 LHR as normal without aggregation, no? Doesn't seem critical to get those in for 10.0.

We could expedite 10.1 a bit (~3 weeks instead of 4-6) and still make the same Chromium cut, I believe. So to avoid CDT having a chrome release of 10.0 instead of 10.1. Is that your concern?

@adamraine
Copy link
Member

Yeah I guess it's not breaking. SGTM

@alexnj
Copy link
Member Author

alexnj commented Mar 3, 2023

I've updated this branch with latest UX recommendations so it should be good for review/merge.

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

Successfully merging this pull request may close these issues.

5 participants