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.

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

Add configurable windows and maximum # of reports to spec #856

Merged
merged 19 commits into from
Jul 17, 2023
Merged

Add configurable windows and maximum # of reports to spec #856

merged 19 commits into from
Jul 17, 2023

Conversation

csharrison
Copy link
Collaborator

@csharrison csharrison commented Jun 22, 2023 •
edited by pr-preview bot
Loading

This PR depends on #855. This PR specifies a partial version of https://github.com/WICG/attribution-reporting-api/blob/main/flexible_event_config.md, i.e. everything except the trigger_specs behavior.

In particular, this allows for a wider range of configurability in the event-level reports, in the form of:

  • Allows specifying anywhere between 1 and 5 reporting windows, with (mostly) arbitrary times
  • Allows specifying anywhere between 1 and 20 total reports

Fixes #736
Fixes #730
Addresses #849 % channel capacity limits

Preview | Diff

@csharrison csharrison marked this pull request as ready for review June 22, 2023 00:39
@csharrison csharrison marked this pull request as draft June 22, 2023 03:42
@csharrison csharrison marked this pull request as ready for review June 22, 2023 14:17
apasel422
apasel422 requested changes Jun 22, 2023
View reviewed changes
Copy link
Collaborator

@apasel422 apasel422 left a comment

Choose a reason for hiding this comment

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

We should also update the header validator and the source JSON schema, though you or I can do that in a followup if you'd prefer.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
apasel422
apasel422 requested changes Jun 22, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@csharrison
Copy link
Collaborator Author

We should also update the header validator and the source JSON schema, though you or I can do that in a followup if you'd prefer.

I'll try to do this in a follow-up. We can also resolve the issue about setting the minimum end duration.

apasel422
apasel422 requested changes Jun 22, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
apasel422
apasel422 reviewed Jun 22, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
@csharrison
Copy link
Collaborator Author

Thanks for the thorough review, Andrew 🕺

@csharrison
Copy link
Collaborator Author

@vikassahu29 can you please review?

@vikassahu29
Copy link
Collaborator

@vikassahu29 can you please review?

Reviewed. LGTM!

@csharrison csharrison mentioned this pull request Jun 22, 2023
Open
@csharrison
Copy link
Collaborator Author

cc @yanzhangnyc

@csharrison csharrison mentioned this pull request Jun 26, 2023
Closed
csharrison and others added 12 commits June 30, 2023 15:52
@csharrison
Spec flexible reports and windows
31cb606
@csharrison @apasel422
Apply suggestions from code review
0b44ad1 
Co-authored-by: Andrew Paseltiner <apaseltiner@google.com>
@csharrison @apasel422
Update index.bs
a4edfc8 
Co-authored-by: Andrew Paseltiner <apaseltiner@google.com>
@csharrison
Suggestions from code review
dcd296b
@csharrison @apasel422
Apply suggestions from code review
b55e056 
Co-authored-by: Andrew Paseltiner <apaseltiner@google.com>
@csharrison
wrapper type
b7fb22c
@csharrison @apasel422
Update index.bs
6e64717 
Co-authored-by: Andrew Paseltiner <apaseltiner@google.com>
@csharrison
Ensure there is at least one window
e95b56a
@csharrison @apasel422
Update index.bs
d9865de 
Co-authored-by: Andrew Paseltiner <apaseltiner@google.com>
@csharrison
1 hour minimum for endDuration
1e32ffd
@csharrison
Rebase report-windows
0d8bcc7
@csharrison
Use parse a duration for start time
52c6cc1
apasel422
apasel422 requested changes Jul 10, 2023
View reviewed changes
Copy link
Collaborator

@apasel422 apasel422 left a comment

Choose a reason for hiding this comment

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

We intend to remove the start time, right?

@csharrison
Copy link
Collaborator Author

We intend to remove the start time, right?

Done

apasel422
apasel422 requested changes Jul 11, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@csharrison @apasel422
Apply suggestions from code review
f037d3c 
Co-authored-by: Andrew Paseltiner <apaseltiner@google.com>
apasel422
apasel422 reviewed Jul 11, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
@csharrison csharrison mentioned this pull request Jul 13, 2023
Closed
apasel422
apasel422 requested changes Jul 13, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
This was referenced Jul 13, 2023
Closed
Closed
apasel422
apasel422 requested changes Jul 17, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
apasel422
apasel422 reviewed Jul 17, 2023
View reviewed changes
index.bs Show resolved Hide resolved
apasel422
apasel422 requested changes Jul 17, 2023
View reviewed changes
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@csharrison csharrison merged commit e3c2036 into main Jul 17, 2023
1 check passed
@csharrison csharrison deleted the flexible-lite branch July 17, 2023 18:25
apasel422 added a commit to apasel422/attribution-reporting-api that referenced this pull request Jul 18, 2023
@apasel422
Allow source expiry and report_window durations to be integers
fa6b44e 
Their values are constrained to the ranges [86400, 2592000] and
[3600, 2592000], respectively, and the corresponding fields in WICG#856 are
integers, not strings.

Part of WICG#888.
@apasel422 apasel422 mentioned this pull request Jul 18, 2023
Merged
apasel422 added a commit that referenced this pull request Jul 21, 2023
@apasel422
Allow source expiry and report_window durations to be integers (#895)
57aae69 
Their values are constrained to the ranges [86400, 2592000] and
[3600, 2592000], respectively, and the corresponding fields in #856 are
integers, not strings.

Part of #888.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apasel422 apasel422 apasel422 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Updating Reporting Windows for Event-level reports in ARA Testing Alternative Reporting Windows
3 participants
@csharrison @vikassahu29 @apasel422