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

spec cumulative-buyer-time and percent-igs-cumulative-timeout #1353

Merged
merged 61 commits into from
Dec 6, 2024

Conversation

morlovich
Copy link
Collaborator

@morlovich morlovich commented Dec 2, 2024

This also fixes some subtleties in the behavior of cumulative timeout, as that ignores things that finish after it rather than interrupt them.


Preview | Diff

@morlovich morlovich added the spec Relates to the spec label Dec 2, 2024
|generateBidDuration|.
1. If |cumulativeTimeoutTracker| [=cumulative timeout tracker/is expired=] then:
1. Increment |metrics|'s [=per participant metrics/cumulative timeouts occurred=] by 1.
1. [=iteration/continue=].
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note the change here; this makes it so the thing affected by cumulative timeout doesn't take effect, which is what.

...And also means it doesn't count as a regular timeout, which you asked about before.

@morlovich morlovich requested a review from qingxinwu December 2, 2024 15:12
@morlovich
Copy link
Collaborator Author

Last one. (Of new stuff, old stuff can be improved)

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
:: A {{long}}, initially 0. Number of interest groups affected by the cumulative timeout
happening.
: <dfn>cumulative buyer time</dfn>
:: A [=duration=] in milliseconds, initially 0.
Copy link
Collaborator

Choose a reason for hiding this comment

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

wondering if it'll be good to duplicate what these metrics mean (as what's in signal base value), or not. Probably be good to have a very short description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I should link all of these to their metrics?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah that'll be good if it's not too much of work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@morlovich morlovich left a comment

Choose a reason for hiding this comment

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

Resolved most things...

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
:: A {{long}}, initially 0. Number of interest groups affected by the cumulative timeout
happening.
: <dfn>cumulative buyer time</dfn>
:: A [=duration=] in milliseconds, initially 0.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I should link all of these to their metrics?

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator

@qingxinwu qingxinwu left a comment

Choose a reason for hiding this comment

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

LGTM % one nit.

spec.bs Outdated Show resolved Hide resolved
@JensenPaul JensenPaul merged commit ea288c4 into WICG:main Dec 6, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Dec 6, 2024
SHA: ea288c4
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to csharrison/turtledove that referenced this pull request Dec 6, 2024
)

SHA: ea288c4
Reason: push, by csharrison

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit to qingxinwu/turtledove that referenced this pull request Dec 9, 2024
)

SHA: ea288c4
Reason: push, by qingxinwu

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants