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

Fix issue 033506: correct droppedEntriesCount #33538

Merged
merged 10 commits into from
May 28, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,22 @@ new PerformanceObserver(callback)
### Parameters

- `callback`

- : A `PerformanceObserverCallback` callback that will be invoked when observed performance events are recorded. When the callback is invoked, the following parameters are available:

- `entries`
- : The {{domxref("PerformanceObserverEntryList","list of performance observer entries", '', 'true')}}.
- `observer`
- : The {{domxref("PerformanceObserver","observer")}} object that is receiving the above entries.
- `droppedEntriesCount` {{optional_inline}}
- : The number of buffered entries which got dropped from the buffer due to the buffer being full. See the [`buffered`](/en-US/docs/Web/API/PerformanceObserver/observe#parameters) flag.
- `options`

- : An object with the following properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe optional here?

Suggested change
- : An object with the following properties:
- : An optional object with the following properties:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we want to do this we should instead use the {{optional_inline}} macro after the parameter name (like this: https://developer.mozilla.org/en-US/docs/Web/API/Request/Request#options).

But, 2 things.

  1. I talked about this a bit in the PR description: "optional" is weird when we're talking about callback parameters. When we use "optional" for a normal API method the semantics is clear: developers don't have to supply a value, and if they don't, some default is used. But with a callback, the developers aren't calling it, so how are developers supposed to interpret "optional"? I think it is probably better, rather than "optional", to describe exactly why/under which circumstances the parameter is not passed, so developers know when they can access it.

  2. Although this parameter is marked optional in the IDL, I can't understand from the spec how it is optional. AFAICS the relevant bit is https://w3c.github.io/performance-timeline/#queue-the-performanceobserver-task, which has:

  1. Call po’s observer callback with observerEntryList as the first argument, with po as the second argument and as callback this value, and with callbackOptions as the third argument. If this throws an exception, report the exception.

I don't see any indication there that options may not be passed. It will be empty, for all but the first callback invocation, based on steps 7 and 8. So what am I missing here?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fair that it will be empty rather than missing. I guess I'm just trying to avoid the original confusion that was raised in #33506 where the user was confused why it wasn't there as they expected it to contain a value every time (where the droppedEntriesCount would be 0 every time after the first—that's not the case). I can understand that confusion. But maybe with all your other changes it's sufficient.


- `droppedEntriesCount`

- : The number of entries which could not be recorded because the {{domxref("Performance")}} object's internal buffer was full.

Note that this is only provided the first time the observer calls the callback, when the buffered entries are replayed. Once the observer starts making future observations, it no longer needs to use the buffer. After the first time, `options` will be an empty object (`{}`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Shoudl this note be de-indented as it applies more to the whole options rather than droppedEntriesCount?

Suggested change
Note that this is only provided the first time the observer calls the callback, when the buffered entries are replayed. Once the observer starts making future observations, it no longer needs to use the buffer. After the first time, `options` will be an empty object (`{}`).
Note that this is only provided the first time the observer calls the callback, when the buffered entries are replayed. Once the observer starts making future observations, it no longer needs to use the buffer. After the first time, `options` will be an empty object (`{}`).

Copy link
Collaborator Author

@wbamberg wbamberg May 20, 2024

Choose a reason for hiding this comment

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

I think it applies more to droppedEntriesCount specifically though. When it says "Note that this is only provided", the referent of "this" is droppedEntriesCount, not options.

And the second sentence is only about droppedEntriesCount as well - we might imagine a future in which other properties get added to options (which is presumably the point of making this an object), and they might have nothing to do with buffering, and then tying this sentence to options as a whole would look wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah agree it's a little ambiguous and kinda applies to both at the moment. It's the last sentence that specifically applies to the options section rather than droppedEntriesCount .

Anyway, let's go with what's there now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree, the last sentence still bothers me a bit here. I would happily omit it, but am OK with it like this as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that the last sentence is a little awkward and would be happy to omit it - it's implicitly true from the other stuff, and has the chance of becoming false if more options are ever added. But I also think it's OK to keep it.


### Return value

Expand Down Expand Up @@ -56,19 +65,20 @@ observer.observe({ entryTypes: ["measure", "mark"] });
### Dropped buffer entries

You can use {{domxref("PerformanceObserver")}} with a `buffered` flag to listen to past performance entries.
There is a buffer size limit, though. The performance observer callback contains an optional `droppedEntriesCount` parameter that informs you about the amount of lost entries due to the buffer storage being full.
There is a buffer size limit, though. The performance observer callback contains an `options` object: the first time the observer calls the callback, the `options` parameter will have a `droppedEntriesCount` property that tells you how many entries were dropped due to the buffer storage being full. Subsequent callbacks will have an empty `options` parameter.

```js
function perfObserver(list, observer, droppedEntriesCount) {
function perfObserver(list, observer, options) {
list.getEntries().forEach((entry) => {
// do something with the entries
});
if (droppedEntriesCount > 0) {
if (options?.droppedEntriesCount > 0) {
console.warn(
`${droppedEntriesCount} entries got dropped due to the buffer being full.`,
`${options?.droppedEntriesCount} entries got dropped due to the buffer being full.`,
);
}
}

const observer = new PerformanceObserver(perfObserver);
observer.observe({ type: "resource", buffered: true });
```
Expand Down