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

Removed aggregation logic. #84

Merged
merged 1 commit into from
May 25, 2018
Merged

Conversation

paulmeyer90
Copy link
Contributor

@paulmeyer90 paulmeyer90 commented May 24, 2018

This change removes the mentions of aggregating reports for now. This will be recommitted on a different branch for further discussion.

See #75 for more info.


Preview | Diff

Copy link
Member

@igrigorik igrigorik left a comment

Choose a reason for hiding this comment

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

👍

@igrigorik
Copy link
Member

Actually, on further thought.. One clarification: personally I don't see any issues with aggregating multiple reports of the same type and reporting them to the origin-nominated reporting endpoint. Specifically, my concern in #81 is the concept of intermediate report collector, which is mentioned as a brief side note in the text.

/cc @dcreager

@dcreager
Copy link
Member

I agree — it would be interesting to try to flesh out a general way to support aggregating reports. It would make the boundary between Reporting and the downstream spec a bit more murky, since it would be the downstream spec that knows whether its report types can be aggregated, and if so, how. (I think the report type → aggregated type mapping that Paul suggested has legs as an answer to that.)

I'm tempted to say that it should be a v2 issue, though. (Which I think is what Paul suggested here.) We've done that for the other proposals to limit upload sizes — rate limiting (#45) and compression (#62).

@paulmeyer90 paulmeyer90 merged commit 0d7ea86 into w3c:master May 25, 2018
@igrigorik
Copy link
Member

I'm tempted to say that it should be a v2 issue, though.

sgtm 👍

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.

3 participants