Skip to content

report: group third-party entities #14655

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

Jump to bottom
Merged
alexnj merged 55 commits into from
Mar 6, 2023
Merged

report: group third-party entities #14655

alexnj merged 55 commits into from
Mar 6, 2023

Conversation

@alexnj
Copy link
Member

@alexnj alexnj commented Jan 9, 2023 •
edited
Loading

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 mentioned this pull request Jan 17, 2023
Merged
26 tasks
@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
@brendankenny brendankenny mentioned this pull request Jan 27, 2023
Merged
1 task
@connorjclark
Copy link
Collaborator

connorjclark commented Jan 28, 2023 •
edited
Loading

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

@alexnj alexnj force-pushed the entity-based-3p-grouping branch from e31c345 to a9439c1 Compare January 28, 2023 06:41
@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
Contributor

adamraine commented Feb 3, 2023

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 •
edited
Loading

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
Contributor

adamraine commented Feb 3, 2023

Yeah I guess it's not breaking. SGTM

@alexnj alexnj mentioned this pull request Feb 3, 2023
Merged
@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.

@alexnj alexnj merged commit 5aca9f3 into main Mar 6, 2023
@alexnj alexnj deleted the entity-based-3p-grouping branch March 6, 2023 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paulirish paulirish paulirish left review comments

@connorjclark connorjclark connorjclark approved these changes

+1 more reviewer

@adamraine adamraine adamraine approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adamraine adamraine

Labels

10.1 waiting4reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alexnj @connorjclark @adamraine @paulirish @devtools-bot