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

Specify Flexible Event behavior other than summary windows #1109

Closed
wants to merge 2 commits into from

Conversation

apasel422
Copy link
Collaborator

@apasel422 apasel422 commented Nov 13, 2023

Instead of comparing the new report's report time to the existing ones' to determine the existing ones' eligibility to be replaced, we consider existing ones eligible if their report time hasn't been reached yet.

It is my understanding that the report-time equality check in the previous version of this algorithm was included with the same intentions.

This has no effect on implementations whose clocks are well-behaved, as report times for a given source will be non-decreasing because trigger times are also non-decreasing, but makes the algorithm more compatible with the proposed Full Flex behavior.


Preview | Diff

@apasel422 apasel422 marked this pull request as ready for review November 13, 2023 19:55
@apasel422 apasel422 added the flexible-event Regarding the flexible event configuration proposal label Nov 14, 2023
@csharrison
Copy link
Collaborator

Can you help me understand how this will work with full flex, especially because in full flex each "trigger" that we sort by priority may not consume any budget?

Regarding the spec without summary_reports, I'm struggling a bit with the proposed algorithm, especially that priority is not considered first. E.g. in the following example:

{
  "max_event_level_reports": 1,
  "trigger_specs": [
    {"trigger_data": [0], "event_report_windows": {"end_times": [3600]}},
    {"trigger_data": [1], "event_report_windows": {"end_times": [7200]}}
  ]
}

with these reports

{"trigger_data": "1", "priority": "200"}
{"trigger_data": "0", "priority": "100"}

We would prioritize trigger data 0 over 1 right? This feels a bit wrong to me, especially if we have some trigger_data that is generally lower priority but with a short window (like a page view conversion).

@apasel422
Copy link
Collaborator Author

Regarding the spec without summary_reports, I'm struggling a bit with the proposed algorithm, especially that priority is not considered first.

I was under the perhaps incorrect assumption that in Full Flex the effective sort key for trigger-application order is the report window ascending, then priority descending, then trigger time ascending. That said, the Flexible Event explainer is quite vague here (#900).

We would prioritize trigger data 0 over 1 right?

With this PR, yes, trigger data 0 would be prioritized.

However, consider the same trigger specs with the following triggers without this PR:

{"trigger_data": "1", "priority": "100"} // trigger time = +1s
{"trigger_data": "0", "priority": "200"} // trigger time = +2s

The first trigger's scheduled report time would be +7200s. The second trigger's scheduled report time would be +3600s. Since the report times are different, the second report will not replace the first one despite having a higher priority and both triggers occurring before either's report time has been reached. This is consistent with today's behavior (Flex Lite and earlier), but could be surprising and is arguably not useful: presumably the reporting origin wants to learn about the earlier-report-window one first, especially since it is higher-priority.

Maybe neither approach is ideal? I certainly want this PR to be compatible with Full Flex even without summary_buckets, so I'll make whatever change you think achieves that here, including leaving the specification as is, but I think we should consider my example and ask if it's the behavior we want.

@apasel422 apasel422 changed the title Allow cross-reporting-window event-level-report prioritization Restrict event-level report replacement using trigger time Nov 15, 2023
@apasel422
Copy link
Collaborator Author

apasel422 commented Nov 15, 2023

Updated this change to only change how eligibility for replacement is determined, not the prioritization logic applied to those eligible, which should give the behavior desired in #1109 (comment) rather than #1109 (comment).

@apasel422 apasel422 marked this pull request as draft November 15, 2023 18:19
@apasel422 apasel422 changed the title Restrict event-level report replacement using trigger time Specify Flexible Event behavior other than summary windows Nov 15, 2023
@apasel422 apasel422 marked this pull request as ready for review November 15, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flexible-event Regarding the flexible event configuration proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants