Skip to content

Sampling interval instead of sampling rate #253

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

Closed
foolip opened this issue Mar 14, 2024 · 8 comments · Fixed by #254
Closed

Sampling interval instead of sampling rate #253

foolip opened this issue Mar 14, 2024 · 8 comments · Fixed by #254

Comments

@foolip
Copy link
Member

foolip commented Mar 14, 2024

In https://groups.google.com/a/chromium.org/g/blink-dev/c/7leKysvPZWk/m/WyFKLqwRAQAJ this aggregated feedback from the Chrome origin trial was provided:

7% of the respondents mentioned that they would prefer to give the callback frequency in time units (secs, milliseconds), similar to other APIs such as setInterval in place of Hz as currently designed. A number scale along with labels would be more useful e.g. { pressure: 2, label: 'nominal' }.

That's is about sampleRate: https://w3c.github.io/compute-pressure/#the-pressureobserveroptions-dictionary

In a look for "frequency", "interval" and "samplerate" in https://github.com/w3c/webref/tree/main/ed/idl I find the following precedent in favor of using a milliseconds interval:

https://w3c.github.io/sensors/ uses a frequency in Hz for how often a sensor should update.

All existing cases of sampleRate on the web platform have to do with audio or video, most of it audio.

I hope that helps make a decision on whether to make a change or not.

@kenchris
Copy link
Contributor

We discussed this before in the team and I don't feel strongly about any of the options. Luckily it is easy to convert from one to the other, but a patch to the spec and implementation is also quite simple.

When I looked at this last I don't think the JS self profiling existed (or I wasn't aware of it) so that balances it a bit in favor of milliseconds.

On the other hand many who use Compute Pressure currently use it with video and audio streams, but I don't think anyone runs it as the same frequency/samplerate.

@rakuco do you have input?

@foolip
Copy link
Member Author

foolip commented Mar 15, 2024

Given that sampleRate is only used for audio/video elsewhere, I might suggest frequency if you want Hz, or sampleInterval if you want a duration in milliseconds. (API owners hat off here.)

@kenchris
Copy link
Contributor

I am fine with changing it to frequency (rename).

The other thing is that we don't expect people to need state changes very often, so mostly in seconds and probably never below one second, so in that sense, sampleInterval might make more sense.

@foolip
Copy link
Member Author

foolip commented Mar 15, 2024

What kind of sampling rates/intervals have you seen people using in demos and prototypes using this API? If it's usually no more than 10Hz, then that would suggest interval as the more convenient/familiar representation.

@miketaylr
Copy link
Member

@andreasbovens - given that Whereby has experimented with this API, any input on naming?

@rakuco
Copy link
Member

rakuco commented Mar 15, 2024

@rakuco do you have input?

I was talking to @arskama about the current implementation, and it actually caps sampleRate at 1Hz (not by design, the code just used to be like that in the original implementation and has not been changed yet), so allowed rates are always in the (0, 1.0) interval at the moment.

Given that there don't seem to be any complaints about this so far, I think it's fair to say that sampling rates are < 10Hz most of the time/all the time, so going for an interval instead of a frequency sounds reasonable.

@andreasbovens
Copy link

At this point, we're using a sampleRate of 1Hz, and talking with our engineers, they feel this naming is clear and makes sense.

However, if there's a desire to change this (and I can see why given the currently allowed interval), sampleInterval with values in milliseconds can work as well.

@tomayac
Copy link
Contributor

tomayac commented Mar 19, 2024

FYI, I have a CL open that changes this accordingly in the article at https://developer.chrome.com/docs/web-platform/compute-pressure.

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 a pull request may close this issue.

6 participants