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

Provide guidelines for mitigation algorithms #241

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

arskama
Copy link
Contributor

@arskama arskama commented Nov 1, 2023

This patch is providing guidelines on numerical values to select for the mitigation algorithms parameters. [1]

[1] #197 (comment)

Fixes: #240


Preview | Diff

This patch is providing guidelines on numerical values to select
for the mitigation algorithms parameters. [1]

[1] w3c#197 (comment)

Fixes: w3c#240
index.html Outdated
<p><i>This section is non-normative.</i></p>
<p>
Based on implementation experience, implementers are advised to apply the mitigation
to a randomized time value within a range between 120000 milliseconds (2minutes) and 240000 milliseconds (4 minutes).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to a randomized time value within a range between 120000 milliseconds (2minutes) and 240000 milliseconds (4 minutes).
to a randomized time value within a range between 120000 milliseconds (2 minutes) and 240000 milliseconds (4 minutes).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ll make the change in my commit. easier

index.html Outdated
Based on implementation experience, implementers are advised to use:
<ul>
<li>
a range in between 300000 milliseconds (5 minutes) and 600000 milliseconds (10 minutes) for |observer|.{{PressureObserver/[[ObservationWindow]]}}.
Copy link
Member

Choose a reason for hiding this comment

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

Editorial nitpick: the |variable| notation is generally used in algorithms. observer has not been defined here. I'd just say something along the lines of "a range [...] for PressureObserver's [[ObservationWindow]] internal slot".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree!

Copy link
Member

@anssiko anssiko left a comment

Choose a reason for hiding this comment

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

Approved with @rakuco's editorial nitpicks addressed.

As a general advise, to make an analogy, informative sections for web specs are similar to comments in code.

What follows is that anything that's not testable should be an informative note.

Copy link
Member

@rakuco rakuco left a comment

Choose a reason for hiding this comment

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

Reading #197 (comment), my impression is that Pete suggested that the spec should have normative ceiling and/or floor values for the intervals described here (this would also help when writing web tests).

Is it really not possible at this point to make any of the values normative?

@arskama
Copy link
Contributor Author

arskama commented Nov 2, 2023

@rakuco

We do have some wpt tests for [[MaxChangesThreshold]] and [[PenaltyDuration]], checking that the random [[maxChangesThreshold]] number choosen is in the range and that penalty is happening during the [[PenaltyDuration]] range.
These 2 internal slots could be normatives.
If they become normative values, can we still keep all the comments around the values selections. (Based on implementation experience[...]. These values are subject to change[...])

the [[observationWindow]] is not as important, it is used to give more randomness to the process, so it can stay non-normative.
About the break calibration, I believe that it can stay as a non-normative section as well.

@rakuco
Copy link
Member

rakuco commented Nov 2, 2023

If they become normative values, can we still keep all the comments around the values selections. (Based on implementation experience[...]. These values are subject to change[...])

I think it's fine to define them normatively and have a note saying the values are based on implementation experience and are therefore subject to change. Changing the values in the future then becomes like any other normative change with user-visible effects, i.e. get buy-in from all implementers (not hard in this case yet since there's just one implementation) and adjust existing web tests.

Copy link
Member

@rakuco rakuco left a comment

Choose a reason for hiding this comment

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

lgtm, but I'd like to see @pes10k's feedback before merging

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@arskama
Copy link
Contributor Author

arskama commented Nov 2, 2023

lgtm, but I'd like to see @pes10k's feedback before merging

Yes, definitely!

@pes10k
Copy link

pes10k commented Nov 2, 2023

I think this approach sounds great, thank you for the work!

@anssiko
Copy link
Member

anssiko commented Nov 2, 2023

Thank You @pes10k for helping us harden this specification further!

@arskama arskama merged commit 1d55881 into w3c:main Nov 3, 2023
1 check passed
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.

Provide better timing guidelines for mitigation algorithms.
5 participants