-
Notifications
You must be signed in to change notification settings - Fork 345
ErrorHandling.md: redo Telemetry with events #196
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
Conversation
ace9dfc to
7ab26f4
Compare
kenrussell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per review on #197: I think it's worth continuing to categorize types of errors so that an application can, in a guaranteed fashion, watch for any out-of-memory errors and attempt to recover from them.
Per the other pull request, only adding fallible Promise-based allocation entry points for buffers and textures doesn't seem a robust enough mechanism.
Sorry for not thinking this through more in our face-to-face discussions.
What do you think?
|
EDIT: moved comment here: #197 (comment) because it makes more sense on that PR |
|
Consider this: OpenGL errors are categorized, and no more than one error of any particular type is reported at a time. Out-of-memory is essentially the only one an app would like to try to recover from. Does it seem a good idea to allow that recovery to be robust - no matter what kind of GPU object was allocated that triggered the OOM? |
|
Thank you @kainino0x for doing this work! It is much easier to digest with 3 separate PRs. TLDR;
partial interface GPUDevice : EventTarget {
attribute EventHandler onvalidationerror;
};
[
Constructor(DOMString type, GPUValidationErrorEventInit gpuValidationErrorEventInitDict),
Exposed=Window
]
interface GPUValidationErrorEvent : Event {
readonly attribute DOMString message;
};
dictionary GPUValidationErrorEventInit : EventInit {
required DOMString message;
};Fire an event named "validationerror" at GPUDevice using GPUValidationErrorEvent with its message attribute initialized to the validation error message. It should include the debug label of the object if possible. Moreover, in your other PRs, you've include some Javascript examples. And I really liked it. Maybe doing the same here could be nice as well. const gpuAdapter = await gpu.requestAdapter({ /* options */ });
const gpuDevice = await gpuAdapter.requestDevice({ /* options */ });
gpuDevice.addEventListener('validationerror', function(event) {
console.log(event.message);
// TODO: Push logs to remote server for telemetry.
}); |
7ab26f4 to
dd3fa8a
Compare
|
Applied your changes, thank you for the input! Not 100% certain that all log entries would be validation errors, but did that rename for now. Can you check the wording for "fires on the main thread event loop"? Not sure how to word it properly. |
dd3fa8a to
dfbd918
Compare
beaufortfrancois
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, did you have a look at https://developers.google.com/web/updates/2018/09/reportingapi?
It may be good to mention it when talking about telemetry.
| (An application may request a new device, or choose to fallback to other content.) | ||
| - `"out-of-memory"`: an allocation failed because too much memory was used by the application (CPU or GPU). | ||
| This includes recoverable out of memory errors that aren't opt-ed in to be handled by the application when the resource was created. | ||
| The `"validationerror"` event always fires on the main thread (Window) event loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to use the existing WebGL task source as defined in https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15 and says something like
The task source for all tasks queued [HTML] in this section is the WebGL task source.
For info, in Picture-in-Picture, I wrote:
The task source for all the tasks queued in this specification is the media element event task source of the video element in question.
For more, check out https://cs.chromium.org/chromium/src/third_party/blink/public/platform/task_type.h
| When there is a validation error in the API (including operations on "invalid" WebGPU objects), an error is logged. | ||
| When a validation error is discovered by the WebGPU implementation, it may fire a `"validationerror"` event on the `GPUDevice`. | ||
| These events should not be used by applications to recover from expected, recoverable errors. | ||
| Instead, the error log may be used for handling unexpected errors in deployment, for bug reporting and telemetry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: validation errors log
dfbd918 to
47af2ea
Compare
|
@beaufortfrancois I have glanced at the Reporting API, but I haven't dug into it enough to understand whether it relates to the kind of reporting we need. Do you know whether we should try to integrate (or merge) this stuff with it? |
|
Reading a little further it seems like:
|
|
Discussed at the 11 Feb 2019 WebGPU Meeting |
|
Merged at the meeting, any further comments can be addressed later |
* fix warnings * Replace usages of constants.ts with `as const`
Completely separates Telemetry from Fallback/Fatal Errors. Simplifies the log entry API to hopefully include exactly what is necessary (although there is a TODO about whether
objectshould be included).