Skip to content

Conversation

@martinthomson
Copy link
Member

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
Member 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.

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
Member 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....)

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
Member 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 <aleiserson@mozilla.com>
@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