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

PerformanceObserver's observe() should support entryType-specific parameters #103

Open
npm1 opened this Issue Jul 23, 2018 · 22 comments

Comments

Projects
None yet
5 participants
@npm1
Copy link
Contributor

npm1 commented Jul 23, 2018

Currently, observe() gets a sequence of entryTypes and optionally a boolean buffered flag. I'd like to augment this as follows:

  • Keep the entryTypes sequence.
  • Allow an optional dictionary for each entryType that can contain entryType-specific parameters.
  • Move the buffered flag to become part of the entryType-specific dictionaries. I believe no browser vendor has implemented this flag yet.

The entryTypes could be used alongside entryType-specific parameters. For example, a user could say:
observer.observe({entryTypes: ["resource"], longtask: {buffered: true, threshold: 100} });

npm1 added a commit to npm1/performance-timeline that referenced this issue Jul 24, 2018

Add entryType-specific parameters for observe()
This PR adds a PerformanceEntryObserverOptions dictionary where entryType-specific parameters are used. The buffered flag is moved to this dictionary. Some <i> tags are changed to <var>. Observe() is updated to consider dictionaries. Queueing an entry is modified to allow passing in a closure. This is in preparation for adding thresholds to certain entry types. Solves w3c#103
@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 7, 2018

So the whole *Observer pattern is that you're supposed to call observe on each thing you observe. So it's not great that we're making observe function take multiple things to observe just in PerformanceObserver.

I think a better interface would be to make observe(~) take "type" and other options directly as in:

observe.observe({entryTypes: ["resource"]});
observe.observe({type: 'longtask', buffered: true, threshold: 100});
@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Sep 7, 2018

We already allow observe to take multiple things to observe. And if there are multiple calls to observe() I think that currently we override what we observe, not append to it.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 8, 2018

The fact PerformanceObserver behaves so differently from other *Observer pattern is bad. We shouldn't make it worse.

We can make it so that we support both modes. When multiple times are specified, simply replace the observed list.

When a single type is specified, it appends or replaces an existing entry of the same type using new options.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Sep 9, 2018

Hmm.. Our earlier proposal was predicated on assumption that folks want to use a single observer to monitor for multiple types, each with potentially different settings. On the WG call, I think we identified that this may not be the best pattern, or pattern we want to encourage. Instead of observing many different types and building a switch statement, create multiple distinct observers.

Given that, WDYT of...

observe.observe({entryTypes: ["resource", "navigation"], buffered: true, threshold: 100});

Observe resource and navigation with same buffered and threshold settings applying to both.

observe.observe({entryTypes: ["mark"], buffered: true});
observe.observe({entryTypes: ["longtask"], buffered: true, threshold: 50});

If you want to observe different types with different settings, create multiple observers? Perhaps we can just stick with entryTypes? Conceptually it's pretty simple to grok and explain, and if we stick with same attributes, we don't need to create any special precedence rules for processing.

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Sep 10, 2018

It seems to me that allowing a single observe() call to specify multiple entryTypes and also various settings is prone to errors if the names of the settings are shared but they mean different things for different entryTypes. Thus, a developer could do:
observer.observe({entryTypes: ["resource", "longtask"], buffered: true, threshold: 100});
intending to limit the amount of longtasks received, but maybe this would have unintended side effects on the resources received if threshold means something for ResourceTiming. So I think I prefer something like allowing either:
observe.observe({entryTypes: ["resource", "longtask"]});
or
observe.observe({entryTypes: ["longtask"], buffered: true, threshold: 100});
That is, we would only support flags when calling observe() on a single entryType. And for this case we can either keep the array format or expect a different kind of dictionary similar to above:
observer.observe({entryType: 'longtask', buffered: true, threshold: 100});

@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 10, 2018

Yeah, I don't think we should use entryTypes to behave differently based on how many types are specified in it. That can be very error-prone with the use of Array.prototype.filter, etc...

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Sep 11, 2018

I don't understand the concern. If you plan to observe multiple entryTypes, you would specify multiple-or-maybe-one and you can use Array.prototype.filter and you wouldn't provide parameters. If you want to observe a single entryType, then you specify it explicitly. Do you have a specific example in mind?

Maybe there's a valid use case of specifying the buffered flag when observing multiple entryTypes so in that sense we can allow flags but then maybe name entryType-specific flags accordingly? For example longtask-threshold. This way, flags that are irrelevant to an entryType can just be ignored even when observe() is called on multiple entryTypes.

I think sticking with always having entryTypes is much cleaner because it lets us have a single observe() method instead of having to specify two: one receiving a dictionary with entryTypes and one receiving a dictionary with type or similar plus other flags.

@toddreifsteck @yoavweiss may also have thoughts on this discussion.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 11, 2018

@npm1 : I'm saying that changing the behavior of observe based on whether there is one item in entryTypes or not is bad because the author scripts can easily reduce what used to contain two items to one by applying a filter. Having entryTypes and type would be fine because then the author is explicitly opting into new behavior.

I would strongly object to add options support with entryTypes.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Sep 12, 2018

@rniwa to clarify, you're saying...

observe.observe({entryTypes: ["resource"]}); // A
observe.observe({type: 'longtask', buffered: true, threshold: 100}); // B
  • (A) should not support any type-specific parameters
    • buffered is OK because it's global?
  • (B) supports custom parameters, because it's explicitly stating what type it applies to
@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 12, 2018

@igrigorik : Sort of. I would prefer if buffered wasn't supported in (A) either. The biggest difference here is that A would replace the list of entry types being observed whereas (B) is additive; e.g. we're not replacing the list of types being observed.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Sep 12, 2018

Hmm, we already spec'ed buffered as a flag on observe. Granted, we don't have any shipping implementations of it yet, so we can back that out.. but I'm less excited about doing that as I suspect this should be a common use case for a lot of monitoring use cases.

E.g. I'm an analytics vendor that, once loaded, wants to suck up all the buffered entries. If we drop buffered that means my script will have to (a) enumerate all the supported types, (b) create an observer for each one with buffered flag set to true. That's a lot of hoops to jump through.

What's your thinking against supporting global flags?

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Sep 12, 2018

Also how do you propose we specify this on the IDL? I played with it and it looks like this is invalid due to both being dictionaries (you can't overload like that):
void observe(PerformanceObserverInit options);
void observe(PerformanceEntryObserverOptions single_options);

@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 12, 2018

E.g. I'm an analytics vendor that, once loaded, wants to suck up all the buffered entries. If we drop buffered that means my script will have to (a) enumerate all the supported types, (b) create an observer for each one with buffered flag set to true. That's a lot of hoops to jump through.

And that analytic vendor doesn't want to write a single line code like this?:
eventTypes.map((type) => observer.observe({type, buffered: true}))

What's your thinking against supporting global flags?

For a better consistency.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 12, 2018

Also how do you propose we specify this on the IDL? I played with it and it looks like this is invalid due to both being dictionaries (you can't overload like that):
void observe(PerformanceObserverInit options);
void observe(PerformanceEntryObserverOptions single_options);

Just have a single dictionary which defines both entryTypes, type, and all other options. We'd then do checks in the definition of observe. See how MutationObserver's observe method does this.

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Sep 13, 2018

A single definition of observe sounds good. In this case, WDYT of having DOMString or sequence<DOMString> entryTypes instead of a separate type?

@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 14, 2018

I don't we want a thing named entriesTypes to change behaviors like that.

@npm1

This comment has been minimized.

Copy link
Contributor

npm1 commented Sep 14, 2018

@igrigorik WDYT? If you're ok with this proposal I can send out a new PR. With the understanding that I plan to add entries such as longtask-threshold to the dictionary.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Sep 22, 2018

E.g. I'm an analytics vendor that, once loaded, wants to suck up all the buffered entries. If we drop buffered that means my script will have to (a) enumerate all the supported types, (b) create an observer for each one with buffered flag set to true. That's a lot of hoops to jump through.

And that analytic vendor doesn't want to write a single line code like this?:
eventTypes.map((type) => observer.observe({type, buffered: true}))

I do suspect this will be a pretty common pattern replicated across many scripts, but fair enough, I don't have any strong objections. @nicjansma @philipwalton any strong feelings on this one?

@npm1 overall, lgtm, I think @rniwa's points all make sense. As an aside, with this pattern we need to land entryTypes to support the pattern above.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Sep 25, 2018

I do suspect this will be a pretty common pattern replicated across many scripts, but fair enough, I don't have any strong objections.

I can believe that. But I think being consistent across *Observer API will improve the overall developer ergonomics of the web platform as a whole. Using the same pattern in many different parts of the platform would reduce the cognitive stress of understanding Web API in general.

@nicjansma

This comment has been minimized.

Copy link

nicjansma commented Sep 29, 2018

No strong feelings. The way we're consuming PO is very context-specific (e.g. different analytics components register their own observer for things they care about), instead of asking for all types at once, or needing to specify multiple entryTypes at once.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Sep 29, 2018

@rniwa yep, fair points.
@nicjansma ty.

Sounds like we have consensus.. Let's make it happen. :)

@yoavweiss yoavweiss added this to the Level 2 milestone Oct 17, 2018

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Nov 27, 2018

Discussed at TPAC F2F:
We have decided that if a user is mixing multiple shapes of this API (so using the old and new shapes of observe on a single observer), the API should throw to discourage developers from doing that. That seems more friendly than to override, which hides the problem and is likely to cause confusion.

npm1 added a commit to npm1/performance-timeline that referenced this issue Dec 7, 2018

Add |type| to PerformanceObserverInit
This change introduces a DOMString type in PerformanceObserverInit to support observing a single entryType with other parameters from the dictionary. The change modifies the observe() method so that:
* observe({entryTypes:...}) is still supported but not with any other member of the PerformanceObserverInit dictionary.
* observe({type: ...}) is now supported, and multiple calls stack.
* Both ways to call observe() cannot be mixed. That is, an observer has to stick to one of the ways, and 'observer type' is introduced to enforce this.

In addition, some nits are fixed along the way, such as using <var> and <a> appropriately throughout.
Solves w3c#103
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment