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

Some followup changes regarding epochs #126

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

martinthomson
Copy link
Member

@martinthomson martinthomson commented Apr 3, 2025

Looking at @csharrison here for a review.

I threw in a fix for #125 with this as a bonus.


Preview | Diff

but retaining browsing history,
a [=user agent=] can [=map/set=] the value
in the [=privacy budget store=] to 0 for all [=epochs=]
for all affected [=sites=].

When clearing site data at the request of a [=site=],
through the use of the [:Clear-Site-Data:] header,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to address the points from #125 (comment)?

However, the only real requirement is that an [=epoch=]
can be used to index the [=maplike=] structures
used for the [=impression store|impression=]
and [=epoch start store|epoch start=] stores.
Copy link
Member Author

Choose a reason for hiding this comment

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

@csharrison suggests that we keep "epoch" as a concept that refers to a fixed span of time. Then we use - in all the algorithms - an "epoch index". Both are interchangeable if you understand the start time for a site, but we get more precision if we are clearer.

@apasel422 apasel422 requested a review from csharrison April 4, 2025 12:11
The start time for a given pair of [=user agent=] and [=site=]
is chosen randomly by the [=user agent=] when an epoch is first needed--
as a side effect of invoking the [=get the current epoch=] algorithm.
The definition of an [=epoch=] is also reset
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The definition of an [=epoch=] is also reset
The [=epochs=] for a given [=site=] will change

To <dfn>get the current epoch</dfn>
given a [=site=] |site|,
and [=moment=] |now|:
and [=moment=] |now|,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we invoke this algorithm with non "now" times (e.g. the clear time), I think we should rename it to something like get the epoch, and change the variable now to moment or something similarly abstract.


To <dfn>get the starting epoch for attribution</dfn>
given [=site=] site:
given [=site=] site,
returning an [=epoch index=]:

1. Let |startEpoch| be a [=user agent=]-defined value
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still confused by this. Let's say my earliest epoch index is 0. Now consider a site that first used PPA 5 years ago, so its epoch start store[0] is 5 years ago. This means I need to support attribution with impressions from 5 years ago right?

Similarly, setting this to a negative number doesn't make sense since there will never be impressions < 5 years old. Setting it to a positive number doesn't make much sense because then sites that just started using the API have to wait arbitrarily long to get attribution, etc.

It seems like |startEpoch| needs to be a function of the epoch start store (which is one of the reasons I was confused about the use of an indexing-based approach in the first place)

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