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

Sketch in last-touch attribution #11

Merged
merged 5 commits into from
Sep 23, 2024
Merged

Sketch in last-touch attribution #11

merged 5 commits into from
Sep 23, 2024

Conversation

martinthomson
Copy link
Collaborator

No description provided.

api.bs Outdated
Comment on lines 475 to 476
To <dfn ignore>fill a histogram using last-touch attribution</dfn>,
given |options|:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method of writing code is super, super annoying. This is far from being ready, but it has the bones that I think we need.

api.bs Outdated Show resolved Hide resolved
api.bs Outdated Show resolved Hide resolved
Comment on lines +639 to +640
1. For each |week| starting from the current week
to the oldest week supported by the [=user agent=]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should define somewhere what a week is. Presumably UTC, but need to choose Sunday or Monday.

Comment on lines +649 to +651
1. Set |impression| to the value in |impressions|
with the most recent |impression|.timestamp.
<!-- TODO define a type for stored impressions -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can make the keys in https://private-attribution.github.io/api/#impression-store-contents linkable, at the cost of the simpledef markup getting even uglier. I tested <dfn lt=stored-timestamp>Timestamp</dfn> before removing it because it was so ugly. Do you have a different kind of definition in mind? Maybe change it to a <dl>?

In the case of major clock adjustments, it is possible that the store could have impressions in the future. Is it worth discussing this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The links we can work out later.

The local machine won't have any issue with that sort of time skew. I hope. (Modulo time adjustments....)

api.bs Outdated Show resolved Hide resolved
Comment on lines +620 to +622
Last touch attribution does not select any impression
that was saved during a week
that does not have sufficient [=privacy budget=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

I had been assuming we could report partial conversion value if there is insufficient but non-zero privacy budget. Since the privacy budget is likely to be chosen by the browser (and may vary between browsers and over time), the conversion site doesn't necessarily know the available budget.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a preference for avoiding that sort of thing, at least until we have discussed it. It might be worth opening an issue to track that though.

Co-authored-by: Andy Leiserson <[email protected]>
@martinthomson martinthomson mentioned this pull request Sep 23, 2024
@martinthomson martinthomson merged commit 1025e0d into main Sep 23, 2024
1 check passed
@martinthomson martinthomson deleted the last-touch branch September 23, 2024 21:31
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.

2 participants