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

Should ReportingObserver.observe accept a filter? #46

Closed
RByers opened this issue Aug 17, 2017 · 9 comments
Closed

Should ReportingObserver.observe accept a filter? #46

RByers opened this issue Aug 17, 2017 · 9 comments
Assignees

Comments

@RByers
Copy link
Collaborator

RByers commented Aug 17, 2017

@tdresser points out that PerformanceObserver.observe accepts a sequence of entryTypes to filter by. I see that MutationObserver, IDBObserver both follow a similar pattern while IntersectionObserver does not.

It seems to me like taking an optional sequence of type strings to filter by is simple, potentially useful and consistent with most of the above and so we should probably just add it. @paulmeyer90 @mikewest @patrickkettner thoughts?

@patrickkettner
Copy link
Collaborator

+1

@RByers RByers self-assigned this Sep 1, 2017
@RByers
Copy link
Collaborator Author

RByers commented Sep 1, 2017

I propose this IDL:

[Constructor(ReportingObserverCallback callback)]
interface ReportingObserver {
    void observe(optional ReportingObserverOptions options);
    void disconnect();
};

dictionary ReportingObserverOptions {
  sequence<DOMString> types;
};

@igrigorik
Copy link
Member

Note that the filtering in PerformanceObserver was also partially motivated by requirement that we didn't want to enable "subscribe to everything" semantics by default, which would enable all monitoring subsystems. Instead, we made an explicit decision to force authors to spell out what they need. My hunch is that you (probably?) want same behavior in this context.

However, this opens another complication: how do you know what's available to monitor?

PerfObserver is specced to ignore unknown types, to allow us to easily extend the list in the future. However, a side effect of that is that it's hard to tell if absence of reports == absence of reports, or lack of support. Hence, based on recent discussions the decision (not specced yet) is to expose a new attribute on timeline to enumerate supported types (see w3c/performance-timeline#77 for background) -- e.g. performance.entryTypes -> Array[type1, type2, ...].

@RByers
Copy link
Collaborator Author

RByers commented Sep 8, 2017

Thanks Ilya!

I've been expecting that all reporting types will be designed to be infrequent (eg. first occurrence of a situation of note, not every one). I.e. reports will correspond roughly to console errors, and it's always a serious bug to have a steady spew of console errors. As such, there should be little harm in allowing (even encouraging) "subscribe to all" semantics, and some benefit - eg. for analytics libraries that want to do all report categorizing and filtering on the server.

If it turns out we do want some sort of heavy type of report in the future, then we could always extend ReportingObserverOptions with explicit opt-in to those types.

The key question is probably whether it's really a good idea to treat the performance overhead of the initial report types (deprecation and intervention) as negligible, and how much value there really is to encouraging/supporting "all". I could be convinced that it's not too valuable and so better to err on the side of following the PerfObserver lead here. Note this is related to the question (in #27) of whether/how to opt into these report types for the HTTP API. The current thinking there was they'd just be in the "default" group and on by default (consistent with the idea that they should be rare enough as to be OK to be included unnecessarily).

WDYT? @mikewest @patrickkettner any thoughts?

@igrigorik
Copy link
Member

What are some examples report types that would be part of initial / default group? My anecdotal experience is that console errors are... very frequent on the web.

That said, for PerfObserver we're designing against a backdrop and goal of supporting ~hundreds of events per second across all current+future emitters, and hence did not want to enable 'on by default'. If we think that ReportingObserver requirements are much lower, perhaps you can get away without these semantics, and/or enable "subscribe to everything" if filter types is unspecified, or some such.

@RByers
Copy link
Collaborator Author

RByers commented Nov 10, 2017

Sorry for the delay. Deprecation and intervention reports are an example that would be part of the default group. Modulo bugs, that should be no more than a couple dozen reports during a page lifetime.

If we're unsure about this I'd be fine starting with a required filter. It's always something we can relax in the future. @patrickkettner thoughts?

@patrickkettner
Copy link
Collaborator

Frankly if I was going to use an ReportingObserver on my site, it would probably be the catch-all (at least in the beginning). However, it being an Observer I would expect the ability to filter like the other *Observers.

I don't have strong feelings either way.

My anecdotal experience is that console errors are... very frequent on the web.

Feel like there may be a miscommunication here. Unless I am missing something, somewhere, no one is proposing that this would be reporting anything on console.error, uncaught errors, etc via this API. Most of the console spam that I see on the web is the result of errors that would be caught using other error services and window.onerror handlers. As a result, few of the errors that you see in the wild today should be showing up, outside of "x has been deprecated and will be removed in version Y of chrome" messages y'all use.

@paulmeyer90
Copy link
Contributor

This API (as proposed) definitely won't be reporting anything on console.error (by default).

Also, I agree that the default behavior of a ReportingObserver should be to catch all report types, with the |types| filter allowing a subset of types to be observed.

@RByers
Copy link
Collaborator Author

RByers commented Aug 1, 2018

Done with ReportingObserverOptions#types

@RByers RByers closed this as completed Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants