这是indexloc提供的服务,不要输入任何密码
Skip to content

Replace aggregatable_expiry with event_attribution_window #577

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

Merged
merged 15 commits into from
Oct 25, 2022

Conversation

johnivdel
Copy link
Contributor

@johnivdel johnivdel commented Oct 6, 2022

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.

Please also update the header validator to remove the aggregatable_expiry field and add the new one.

@csharrison
Copy link
Collaborator

Two points:

  1. We should consider making event_attribution_window extensible enough that it could take a begin and end time. This satisfies the use-case of partitioning users/sources across a lookback window (e.g. some sources look at 1-7 days, some only at 7-14, some only at 14-30). I don't think it needs to be in this PR but we should consider this extension for the future (e.g. you can input either a scalar or a tuple here).
  2. Don't we also want an aggregate_attribution_window?

johnivdel and others added 2 commits October 10, 2022 16:35
Co-authored-by: Andrew Paseltiner <apaseltiner@google.com>
@johnivdel
Copy link
Contributor Author

Two points:

  1. We should consider making event_attribution_window extensible enough that it could take a begin and end time. This satisfies the use-case of partitioning users/sources across a lookback window (e.g. some sources look at 1-7 days, some only at 7-14, some only at 14-30). I don't think it needs to be in this PR but we should consider this extension for the future (e.g. you can input either a scalar or a tuple here).
  2. Don't we also want an aggregate_attribution_window?

I think that is reasonable, but I don't think using a single value will make swapping to a tuple or similar too difficult, so I would prefer to err on the side of YAGNI.

  1. Good question, the issue in question seems to only request the capability for event level reports, which is why I only added an event window param. I think we could add one for aggregate to make this more flexible. That being said, do we have a use-case where you would want to stop generating aggregate reports and only generate event reports?

@johnivdel
Copy link
Contributor Author

Please also update the header validator to remove the aggregatable_expiry field and add the new one.

Done, PT another look.

@csharrison
Copy link
Collaborator

  1. I think that is reasonable, but I don't think using a single value will make swapping to a tuple or similar too difficult, so I would prefer to err on the side of YAGNI.

That's fine with me.

  1. Good question, the issue in question seems to only request the capability for event level reports, which is why I only added an event window param. I think we could add one for aggregate to make this more flexible. That being said, do we have a use-case where you would want to stop generating aggregate reports and only generate event reports?

I was thinking of use-cases that would use this capability and also want consistency with aggregatable reports. For instance, the use-case in #561 might take advantage of this for both reports, where we have privacy restrictions about small expiry values but no privacy restrictions for these windows because they take effect after attribution matching.

@johnivdel
Copy link
Contributor Author

  1. I think that is reasonable, but I don't think using a single value will make swapping to a tuple or similar too difficult, so I would prefer to err on the side of YAGNI.

That's fine with me.

  1. Good question, the issue in question seems to only request the capability for event level reports, which is why I only added an event window param. I think we could add one for aggregate to make this more flexible. That being said, do we have a use-case where you would want to stop generating aggregate reports and only generate event reports?

I was thinking of use-cases that would use this capability and also want consistency with aggregatable reports. For instance, the use-case in #561 might take advantage of this for both reports, where we have privacy restrictions about small expiry values but no privacy restrictions for these windows because they take effect after attribution matching.

If they want consistency, they would be able to set expiry == event_report_window. Do we think a reporter would ever want to:

  1. set aggregate_report_window < event_report_window
  2. set both aggregate_report_window and event_report_window < expiry?

@johnivdel johnivdel requested a review from apasel422 October 20, 2022 22:05
@csharrison
Copy link
Collaborator

If they want consistency, they would be able to set expiry == event_report_window. Do we think a reporter would ever want to:

  1. set aggregate_report_window < event_report_window
  2. set both aggregate_report_window and event_report_window < expiry?

(2) is required to (partially) address #561 where we'd want consistency and the window < expiry.

Co-authored-by: Andrew Paseltiner <apaseltiner@google.com>
@johnivdel johnivdel marked this pull request as ready for review October 25, 2022 16:51
@johnivdel johnivdel merged commit a271750 into main Oct 25, 2022
@apasel422 apasel422 deleted the swap-expiry-for-window branch October 26, 2022 16:30
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.

4 participants