Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.58k stars 797 forks source link

Ads module: Initialize lazily #14777

Closed dero closed 4 years ago

dero commented 4 years ago

Is your feature request related to a problem? Please describe.

The Ads module is inefficiently loading ads assets (pubmine.com scripts etc.) even when there are no ads displayed on the current page. The module should be able to detect the absence of ads on any page and skip the initialization step.

Context

There are a few ways theme / plugin authors and content editors can add ads to the website:

Describe the solution you'd like

Update some of the back-end initialization steps (insert_head_meta, insert_head_iponweb) and initialize the ad front-end experience lazily instead.

Consider that the ad units can either be placed to the page on an initial render OR retrieved dynamically (e.g. through REST endpoint) and placed to the page later dynamically. The units' markup contains __ATA.cmd.push() calls that allows the pubmine.com platform to observe new units added to the page regardless of how they're added.

A proper solution might be updating the init code in insert_head_meta and writing a custom __ATA.cmd.push method to dynamically initialize the full experience when this first happens. That way it doesn't matter what the source of the ad unit markup is, the experience would always be initialized lazily when appropriate.

Using this architecture, the JS would not be initialized on pages that don't display any ads. This could alleviate some of the issues associated with the overhead introduced by 3rd party ad network scripts (e.g. https://github.com/Automattic/jetpack/issues/7583) and would allow website authors to optimize certain pages for performance - e.g. landing pages, homepage etc.

Explored a simpler alternative solution

I believe every ad unit uses the [WordAds]->get_ad(...) method to render appropriate markup. The idea is that only the first get_ad call would insert all the JS init code. This logic can run on the back end, but wouldn't help in situations when ads are injected dynamically on the front-end.

jeherve commented 4 years ago

cc @dbspringer; could you weigh in here?

dbspringer commented 4 years ago

I think there's definitely room to improve how/where we're loading the ads loading scripts, but I'm dubious that lazy loading approach is the best fit for what we need. The challenge the ads module faces are 1) the ad loading framework needs to be loaded at page creation time for various reasons (e.g. header bidding kickoff, brand safety checks, etc) and 2) we don't necessarily have a priori knowledge of an ad unit that could but isn't going to be loaded.

There are definitely a few "whole cloth" checks we should do ahead of time to ensure the framework is only loaded when needed (e.g. check that posts/pages/archives are enabled, the header unit, or a widget is registered for that particular page view). We've tried in the past to do a mix of dynamic loading like you describe, but the edge cases and permutations of potential units has made the efforts break down.

I'd be interested in a demo if what you're proposing, I definitely admit that after looking at the problem so long I might be stuck in looking at the problem in a particular way.

dero commented 4 years ago

Thanks for the follow up, @dbspringer.

the ad loading framework needs to be loaded at page creation time for various reasons [...]

I'd like to understand those reasons in more detail. Is there a documentation for the ata.js script anywhere? (Or an non-minified script?) If there are vital side effects that need to run every page load regardless of the units on the page then I agree the proposed solution probably can't work without significant refactoring of the ad framework itself.

I did some light testing and I can see the payload sent to http://s.pubmine.com/adjr contains header bidding data, defined slots information and various metadata related to the current page and session. This is equivalent to how other HB / ad systems work and it's in theory not opposed to lazy initialization.

I'd be interested in a demo of what you're proposing [...]

Will do. :+1:

dbspringer commented 4 years ago

Let me reach out to our ad partners and verify that they still need the scripts to load they way they had been in the past--it's honestly been a few years since I've checked in about it and the framework has updated a few times in the mean time.

dero commented 4 years ago

@dbspringer Added a quick and dirty demo here: https://github.com/Automattic/jetpack/pull/14812

dbspringer commented 4 years ago

Howdy @dero, I had a discussion with our ad partner and wanted to give you a summary.

Is there a technical reason we have to load the ad framework as early as possible? Short answer: no. Longer answer: “regular” ad units should load fine, but extras like the sticky ads won’t work.

What are the potential downside to lazy loading? Each second we delay the loading of the framework cascades into delays in header bidding, brand safety, and viewability; ultimately reducing earnings ~7% per second.

What can we do to test/fix this? It's possible to try it out, but we're concerned it might tank earnings and we would want a measured approach. We would want to do an A/B test on a non-trivial ads site where we could add another configuration flag about which approach we were taking and we could track the earnings across them. Unfortunately we probably don’t have the resources to do all of this and additionally…

How big of a problem is this really? We performed a brief investigation into the number of page views wherein we know the ad framework loaded, but no ad calls were made — a quick search found no instances of this occurring in the past couple days.

My gut tells me this isn’t a very common occurrence generally–it's unlikely someone would enable the module, but then disable most every potential instance.

There's also the potential that you might might not be seeing ads on a page where the scripts are loaded because there was no inventory for those units--they'll automatically collapse if there's nothing to show.

But what can we do in the mean time? We can add a few more checks (e.g. no header or widget registered and it’s an archive view with ads disabled on archives) to be ~80% certain an ad should not load on a particular page; this should reduce the theoretical burden even further.

What about long term? Our partner team is currently working on a version of our framework that pushes a lot of the loading/work from the client to our back-end, reducing the client burden to negligible levels.

Conclusion My initial response is accurate in practice (though not technically). We're aware of the issue, but that we don’t feel confident in making the change without some extensive testing first. We’ll look into adding some additional checks and it’s probable that it won’t be a burden in the future with the new loader update.

dero commented 4 years ago

Hi @dbspringer, thanks for the detailed response. :+1:

It sounds to me that due to this not being a wide-spread issue and due to the (both revenue and functional) risks associated with changing the loading strategy, it may make sense to prioritize other performance enhancement efforts - especially if your partner team is moving most of the logic to the back-end eventually.

If you agree, feel free to close the issue.

If you decide we should optimize in the mean time (by detecting enabled placements, widgets and shortcodes), I'm happy to help out and attempt a solution.

kraftbj commented 4 years ago

After conversing with @dbspringer, we agree with your summary @dero. Marking it as a wontfix, but definitely something to consider in the future when the underlying situation is more favorable.