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

PressureObserver options object: constructor vs observe method #256

Closed
Elchi3 opened this issue Apr 1, 2024 · 3 comments
Closed

PressureObserver options object: constructor vs observe method #256

Elchi3 opened this issue Apr 1, 2024 · 3 comments

Comments

@Elchi3
Copy link

Elchi3 commented Apr 1, 2024

Currently, sampleInterval is an option of the PressureObserver constructor. If I want to create several pressure observers, I guess my code would look like this:

const cpuObserver = new PressureObserver(callback, { sampleInterval: 1500 });
await cpuObserver.observe("cpu");

const cpuObserverHighSampleRate = new PressureObserver(callback, { sampleInterval: 7000 });
await cpuObserverHighSampleRate.observe("cpu");

const gpuObserver = new PressureObserver(callback, { sampleInterval: 2500 });
await gpuObserver.observe("gpu");

In other APIs, for example for the PerformanceObserver, the options are on the observe method. If that would be the case for this API, I could write code like this:

const observer = new PressureObserver(callback);
await observer.observe("cpu", { sampleInterval: 1500 });
await observer.observe("cpu", { sampleInterval: 7000 });
await observer.observe("gpu", { sampleInterval: 2500 });

Not sure if having multiple pressure observers is typical but I guess if I had multiple of them, I likely want to have different sources or options. I see that "cpu" is the only source and sampleInterval is the only option for now, though, so maybe this will only matter if this API offers more sources and more options in the future.

Even more future proof:

await observer.observe({ sources: ["cpu"], sampleInterval: 2500 });
@arskama
Copy link
Contributor

arskama commented Apr 2, 2024

Indeed, the plan is to have more than one source in the future.
Also as you mentioned, in issue #213, the array of source was already proposed.
We will be adding sources and fixing #213 whenever we find reliable enough metrics to read for these sources.

@kenchris
Copy link
Contributor

kenchris commented Apr 2, 2024

I believe we used to have the options as part of the observe method, and I don't recall the reason why we moved it. Maybe for consistency with other API at that point.

I am not against moving it to the observe method. Potentially we could also do that later and have it overwrite what was set in the constructor.

@Elchi3
Copy link
Author

Elchi3 commented Apr 5, 2024

Looks like this will be fixed by #261

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

No branches or pull requests

3 participants