Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP API #1
WIP API #1
Changes from 1 commit
57e1818
891ab30
641ac8f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The write-only piece is negotiable. I can see why a site couldn't modify its store of impressions, but it's also not really necessary if the reading by other sites is controlled properly.
The whole business with "what does the e-Privacy Directive say about storing information" is silly. It matters, of course, but not every regulatory system will grow a PECR-like piece of baggage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
write-only
term was actually in the original and I also had some doubts about it. I think it's an oversimplification. We should discuss the characteristics of the store somewhere (I think I added a section for that), but it takes more than just saying "write only" to capture the important characteristics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Work out whether a PrivateAttributionAggregationSystem (wow, right name, hard to type) needs any additional attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that we can omit the
PrivateAttribution
prefix from these definitions? I'm not sure where all they might flow to and whether there is namespacing other than from the prefix.I ask because I think
PrivateAttributionImpressionOptions
needs to be further refined as something likePrivateAttributionImpressionOptionsLevel1
(to specify the bag of attributes for the level 1 API), which is really quite a mouthful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have a bucket of fields, some of which are mandatory, let's move the aggregation system into the bucket.
Except... We don't need to know which system is being used, do we? I don't think we care: budgeting happens at the point of conversion, so we probably don't care where conversion data is sent as long as the aggregation system is trustworthy. But at impression time, we really don't care. Yet. In the full API we will. But the bag of attributes method lets us solve that problem neatly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't strictly need it here in the early binding case, but given that we expect to have it eventually, it seems simpler to include it. It seems better to encourage users to register an impression for each aggregator than to set a precedent of supporting the model where the same impression can be referenced by multiple different aggregators at conversion time. There isn't a lot of complexity in the impression data right now, but what happens if the aggregators want to set conditions (like a limit on impression TTL, or the structure of ad IDs)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, I don't think that this is a string that someone will be happy typing often:
That's already unwieldy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about shortening it to
aggregator
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things on this that are somewhat in tension:
However, I think that the resolution of that tension is in the MPC (DAP specifically). We might want to ask aggregation service operators to limit histogram size as some function of the number of reports they aggregate. (I still somewhat like the idea of refusing to return a value if an aggregate is smaller than some threshold, even though it is perverse from a strictly DP interpretation perspective.) If the aggregator supports more, then the API should not be where limits are applied. Also, different browsers will have different views on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're using DAP, then doesn't the browser need to know the histogram size to construct the report?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'll need to know what size to make. We'll ask the site to give us the size they want.
But we don't need to validate or limit that. If it is the wrong size, that's their problem.