Feature Request: Add removeEventListener/unobserve equivalent

[patrickhulce]

Currently every invocation creates a new PerformanceObserver that never unobserves so there's no way to remove a handler. It'd be nice to be able to remove old listeners.

EDIT: This affects the web vitals extension which leaks listeners on every activation of tab. I'm fixing over there, but this seems generally useful too 😃

[philipwalton]

I didn't add an unobserve/disconnect method for a few reasons:

• We don't want to encourage folks tracking these metrics by slightly different definitions (e.g. CLS prior to load, etc.)
• When bfcache ships in Chrome we're going to need to re-invoke the user callbacks on cache loads, and since (in general) developers are less familiar with nuances of bfcache, I didn't want them to remove listeners prematurely—thinking the event have already happened so they're no longer needed.

We could probably disconnect the LCP and FID listener because it's looking less and less likely that the API is going to change to re-emit those entries (a different name will probably be used), but I suspect the one causing the extension problems in CLS, and I don't want to change the API to allow that.

Out of curiosity, why is the extension creating new listeners with the activation of each tab?

[patrickhulce]

We don't want to encourage folks tracking these metrics by slightly different definitions

Interesting I would've expected that to be accomplished with documentation rather than an inability to ever remove it. In my experience developers tend to forget to remove listeners anyway (like the bug in web vitals extension ;), so I would've been surprised to folks using it on their own if it's discouraged in examples and just a silent .unobserve method sitting on the API.

since (in general) developers are less familiar with nuances of bfcache

You can definitely say that again 😆 I personally have no idea how bfcache works. If you don't mind me asking how does this impact things? Is the concern that page removes the listener onbeforeunload/pagehide? And does bfcache preserve the v8 isolate and resume it once the user clicks back without reexecuting anything? 😮

Out of curiosity, why is the extension creating new listeners with the activation of each tab?

I think it was just an oversight. I found it when looking into why it was pegging my CPU usage at 100%. I don't fully understand the architecture of the extension to answer for them but I'm sure the original authors had good reasons :)

[patrickhulce]

@brendankenny linked me to the bfcache design doc, and I think I have more questions now then when I didn't know how it worked 😆

From the page lifecycle it seems like bfcache will just be identical to page freeze in which case why would anyone want to count it as a separate navigation and have their same listeners reinvoked? It seems like it would actually be very desirable to be able to unobserve once a page goes into freeze to avoid having uninformative instant 0ms LCP timings reported for what is basically someone hitting the back button and the page coming out of hiding instead of an actual navigation. Am I missing something here?

[philipwalton]

Sorry for the slow reply!

From the page lifecycle it seems like bfcache will just be identical to page freeze in which case why would anyone want to count it as a separate navigation and have their same listeners reinvoked?

I agree it seems a bit counter-intuitive, but here's the basic argument.

1. bfcache improves the page load experience for users because those pages appear to be loaded instantly, and bfcache does this by restoring those pages from memory rather than reloading them over the network.
2. However, this now means that most sites will have fewer page loads in their datasets, and the removed page loads would likely have been relatively fast (assuming some amount of caching is used), which will skew the distribution.
3. The result is that, even though bfcache will make load feel faster for users, enabling it in Chrome will likely result in an increase in LCP and FID at p75—just because there are now fewer total page loads in the dataset.

To address this concern (as a temporary solution until proper bfcache-measuring APIs are developed), CrUX will report a zero value for both LCP and FID for all bfcache loads. While obviously not perfect, the thinking here is that the likelihood of a bfcache load actually being above the 75th percentile is so small, it's an acceptable tradeoff.

The goal is to try to measure—as accurately as possible given the APIs we have—what the 75th percentile of these metrics is for real usage.

Does that make sense?

[patrickhulce]

That certainly makes sense from the CrUX stakeholder "don't move the goalposts" perspective 👍

As a site owner though, I'm not sure I share this motivation in my own measurements. I'm not interested in my site's performance as a measure of how frequently my users hit the back button :) That's certainly a separately useful insight, but being forced to count this as a separate navigation via the canonical web-vitals API still feels odd. Knowing this, the first thing I'm going to try to do when analyzing my site's performance is going to be excluding these 0s skewing the distribution and anyone that isn't aware of this distinction is probably going to get a falsely rosy picture of their performance because of how counterintuitive it is.

The rabbit hole of "constructing a site that encourages back button interaction to goose CrUX numbers" is probably something not worth exploring here but does add another arrow in my "RUM is too complex to boil down to aggregate thresholds" quiver 😆

[philipwalton]

As a site owner though, I'm not sure I share this motivation in my own measurements. I'm not interested in my site's performance as a measure of how frequently my users hit the back button :)

It's possible that pages can do things that make them ineligible for bfcache, and if they do, users will experience what feels like slower loads (from their perspective). If none of the metrics we're promoting account for this, then developers may not be aware of the real experiences users are having.

That's certainly a separately useful insight, but being forced to count this as a separate navigation via the canonical web-vitals API still feels odd.

The goal of the web-vitals library is to enable RUM for sites in a way that matches CrUX as closely as possible (in fact, the original name for the library in the design doc was CrUX.js). Since Chrome and CrUX have chosen to measure bfcache loads this way, I want to continue to align with that. (If you still feel this measurement is the wrong approach, you could raise the issue on blink-dev)

Knowing this, the first thing I'm going to try to do when analyzing my site's performance is going to be excluding these 0s skewing the distribution

I think this is totally fine, and I'll likely do the same. Also long as there's some metadata in the callback that makes it clear a particular metric instance came from a bfcache load, you'll be able to annotate the data you send to your analytics back end—allowing you to report on load performance with and without these entries.

I don't think is that much different from a site that adds offline-first service worker caching to make their pages load instantly on repeat visits, which (AFAICT) most people still track as "real" page load in their analytics, since it's what the user is actually experiencing.

anyone that isn't aware of this distinction is probably going to get a falsely rosy picture of their performance because of how counterintuitive it is.

I want to push back here a bit and suggest that this can only be interpreted as a rosy picture if you're focusing on traditional page load performance, but the teams working on Web Vitals at Google are moving more and more away from "page load" and more and more toward a concept of "page visit" or "page session"—we've just not yet identified the right normalization and delineation strategies (e.g. should we delineate any time an SPA updates the URL? How about when a user backgrounds the tab?)

bfcache is one of the more clear-cut delineation candidates, so it was an easier decision to go ahead with it, but we expect to do more of this kind of thing in the future, we just need to make sure it's not easily game-able (as you also point out).

[patrickhulce]

If none of the metrics we're promoting account for this, then developers may not be aware of the real experiences users are having.

Very good point, I hadn't really offered an alternative for how to still incentivize these improvements. I'll get on that :)

Also long as there's some metadata in the callback that makes it clear a particular metric instance came from a bfcache load, you'll be able to annotate the data you send to your analytics back end—allowing you to report on load performance with and without these entries.

True! I must've missed it but how are these annotated right now such that it's clear and obvious to the listener it's a bfcache navigation? Is CrUX going to separate these out into a separate bucket so page loads can continue analyzed without these mixed in? Maybe that's all I'm really looking for.

I don't think is that much different from a site that adds offline-first service worker caching to make their pages load instantly on repeat visits

I think we have different assumptions about the user intent behind hitting the back button then 😆 Perhaps my usage pattern is atypical (if there's any data that was analyzed for user habits with the back button I'd love to learn from them! :D), but hitting back for me is almost always because of an accident where I didn't mean to go somewhere else, e.g. more of a continuation of my last visit and not a new, separate visit. An intentional repeat visit from my perspective is then in a totally different class and deserves to be counted as a separate navigation (along with all the rewards of delivering a high-performing warm load experience).

the teams working on Web Vitals at Google are moving more and more away from "page load" and more and more toward a concept of "page visit" or "page session"

As one should, that's all great. I guess my core beef at the end of the day is it feels like they keep being mixed into the same bucket with "page loads" without any distinguishing characteristics in CrUX. This makes "page loads" more and more difficult to analyze when the bucket is mixture of very different user journeys.

[philipwalton]

Sorry again for the slow reply! (Believe it or not, I've been out of the country...)

I must've missed it but how are these annotated right now such that it's clear and obvious to the listener it's a bfcache navigation?

Well, the bfcache changes aren't currently in the library (and I'm not 100% sure how I'm going to expose this info), but what I am definitely planning to do is update the Metric.id value, so these metric instances can be separated from the metrics reported prior to the page entering the bfcache.

At a minimum, developers could self-annotate by adding their own pageshow listener:

let isBFCacheLoad = false;

addEventListener('pageshow', (event) => {
  if (event.persisted) {
    isBFCacheLoad = true;
  }
});

I could potentially add something like isBFCacheLoad to the Metric object, but I want to be careful not to do something too specific to bfcache, since it's entirely possible we do something similar for SPA loads or restores from long-backgrounded pages as well.

Is CrUX going to separate these out into a separate bucket so page loads can continue analyzed without these mixed in? Maybe that's all I'm really looking for.

Hmmm, I don't think so?

I guess my core beef at the end of the day is it feels like they keep being mixed into the same bucket with "page loads" without any distinguishing characteristics in CrUX. This makes "page loads" more and more difficult to analyze when the bucket is mixture of very different user journeys.

I can understand that, but I think the intention behind CrUX was to be a reflection of the user experience and not to be a reflection of page load performance (cc: @rviscomi in case he has any thoughts).

For page load performance, specifically, RUM tools should ideally be granular enough to allow segmentation of all Web Vitals metrics by "visit type" (or whatever it ends up being called).

[patrickhulce]

Believe it or not, I've been out of the country...

Woah! I have more questions and fascination to hear these tales of adventure than anything else here 😄 But probably best saved for another medium. Back to it...

At a minimum, developers could self-annotate by adding their own pageshow listener:

I feel like this thread has drifted a little from the original intent, so I'll try to refocus here :) I'm concerned that developers can be trusted enough to know they have to do this in order to fully understand the output of this library but they can't be trusted enough to know how to use a removeEventListener function with documentation 😕

Even if it were to simplify everything with bfcache I'm still incredulous that some form of removeEventListener/unobserve is bad 😆 There are just many cases where a developer needs the ability to juggle and/or remove event listeners they have installed to avoid memory leaks. I definitely see someone wrapping this in a react useEffect hook and getting lots of accidental copies beaconed unnecessarily. At the very least if this is a hard line in the sand and the official position is that removing the installed listeners should be impossible with this library then IMO the docs should come with a big warning to not call these functions more than once.

Hmmm, I don't think so?
CrUX was to be a reflection of the user experience and not to be a reflection of page load performance
RUM tools should ideally be granular enough to allow segmentation of all Web Vitals metrics by "visit type"

Is it accurate to say then from your perspective that CrUX is not a RUM tool? Or at least not trying to be a good RUM tool even if it can somewhat serve as one? I'm not looking for a particular answer here, it's just very helpful to know that there is not alignment on this position of what CrUX is across CWV efforts so we can discuss from a place of shared understanding.

[philipwalton]

I feel like this thread has drifted a little from the original intent, so I'll try to refocus here :)

Yes, let's refocus :)

I'm concerned that developers can be trusted enough to know they have to do this in order to fully understand the output of this library but they can't be trusted enough to know how to use a removeEventListener function with documentation 😕

I wouldn't say my concern is that they can't be trusted to use a removeEventListener function with documentation, my concern is that I'm not convinced there's a strong enough need to have such a function.

For FCP, FID, and LCP, the library already disconnects the PerformanceObserver once the final value is known, but it doesn't let users unobserve the callback since we'll likely need to invoke it again after a bfcache load. And for CLS we don't actually want people to disconnect the observer, since if they do they'll be measuring CLS incorrectly.

(EDIT: hmm, looks like only FID disconnects the observer, but that's a bug, I'll update the others to do so too)

I definitely see someone wrapping this in a react useEffect hook and getting lots of accidental copies beaconed unnecessarily. At the very least if this is a hard line in the sand and the official position is that removing the installed listeners should be impossible with this library then IMO the docs should come with a big warning to not call these functions more than once.

Sure, I can see how this could be misused, but presumably such a situation would be a mistake, and adding an unobserve function wouldn't necessarily prevent mistakes like that. Also, I don't think there's a valid use case for calling these functions and then disconnecting all of them in a useEffect hook.

All this being said, I'd be happy to add clarifying language indicating that these functions should not be called more than once per page load. (And we'll definitely need clarifying language once the bfcache functionality is added.)

In the end, I want this library to just "do the right thing" as much as possible since we've learned over and over again from lots of experience that people don't thoroughly read the documentation, and there are lots of edge cases that are easy to overlook. For example, with LCP it's clearly documented in the code sample that people measuring LCP should ignore cases where the tab was loaded in the background, yet 100% of the RUM libraries I've audited since we announced Web Vitals have missed this. And these are not random developers, these are folks whose business is building RUM products. So, as a results, I've chosen to keep the API surface for the library as small as possible.

[patrickhulce]

I'll update the others to do so too

Awesome 👍

I'd be happy to add clarifying language indicating that these functions should not be called more than once per page load.

I want this library to just "do the right thing" as much as possible

Great 👍 In this case I would also encourage tracking internally to only allow a single global installation of the listeners if the intent is that it should never be called more than once. "people don't thoroughly read the documentation" ;) and having a function that is never supposed to be invoked more than once but is called getX feels like a recipe for accidental misuse. I have a hunch that the author in the CWV extensions situation believed that it was simply getting the value and not installing new listeners that would grow forever (which it sounds like you already agree that part was a bug that will be fixed 👍 ), which a name like getLCP certainly suggests to me without any other knowledge about how the library works.