Prebid core: automatic dynamic loading of debugging module #8106

dgirardi · 2022-02-23T01:05:27Z

Type of change

Feature

Description of change

This adds a "standalone" version of the debugging module to the build, and automatically loads it when debugging configuration is passed to setConfig or loaded from session storage. All other debugging logic is moved to the "debugging" module.

By "standalone" I mean a version that does not rely on having a specific build of "prebid-core.js" available, as is the case for all other modules; this is to enable it to work with third party builds (such as any that was done by consuming Prebid from npm).

Rothalack · 2022-03-31T19:45:40Z

I'm not opposed to this change. It appears to be working for me in testing. I have apprehensions of the wider implications of this change, but that's mostly because I don't feel very confident of my understanding of what other implications it may have. I don't want to be the final say on that. Would like more eyes on it.

robertrmartinez · 2022-03-31T20:21:44Z

@ChrisHuie I do not think I have enough time to get to this this week unfortunately. Sorry for the lag.

I can try and get back to this one middle to end of next week

ncolletti

Nice work here, the PR looks good as-is and my tests are passing. However I share the same sentiments as the previous reviewer that it may be best to have another look from a reviewer who may be more knowledgeable of edge cases to consider before introducing these changes.

ncolletti · 2022-04-21T03:57:36Z

modules/debugging/WARNING.md

+Because of this, **this module cannot freely import symbols from core**: anything that depends on Prebid global state (which includes, but is not limited to, `config`, `auctionManager`, `adapterManager`, etc) would *not* work as expected. 
+
+Imports must be limited to logic that is stateless and free of side effects; symbols from `utils.js` are mostly OK, with the notable exception of logging functions (which have a dependency on `config`).


If there are no import guards that can be setup at build time, then this may be something to mention in the PR_REVIEW doc for others to be on the look out for.

patmmccann · 2022-05-11T12:24:29Z

@allanjun this should integrate nicely into tools, we're very interested in your feedback

allanjun · 2022-05-11T13:50:23Z

hi @patmmccann thanks for tagging me. Maybe I didn't understand it right, but does this mean we need publishers to also host the file debugging-standalone.js ? Or we plan to host this at jsdelivr ? Should this remote file url be configurable at runtime via setconfig ?

dgirardi · 2022-05-11T17:28:12Z

@allanjun, debugging-standalone is pulled from jsdelivr by default, configurable at build time - the intent is to allow debugging of production sites without any extra setup work (and without the weight of debugging code until it's needed).

I decided to not make it configurable at runtime because to get the effect of "reload the page to see your mock ads" the URL would need to be stored locally (in many cases we will need it before any call to setConfig). That's essentially the same as doing eval(localStorage.sourceCode) - it's a potential security issue.

We could allow a runtime setConfig({debugUrl}) that would work only if it's called before requestBids - that is very similar to a build time config, because it needs to be set pre-emptively and changing it requires re-deployment. But if the interface is nicer I can make the change.

allanjun · 2022-05-12T13:36:54Z

thanks for the explanation @dgirardi all makes sense. Professor Prebid could ensure that setConfig runs before requestBids (so would be for the other hooks that intercept the bidResponses) but I think that won't be needed, the debugging file on jsdlvr would do enough. We can include that in the future if really needed.

dgirardi · 2022-05-12T17:08:19Z

@allanjun I am not that familiar with what extensions can do nowadays, but I'd be surprised if that's true in general - setConfig and requestBids could be bundled together in one javascript file and run before you get control back. Even a pre-set pbjs.que could technically be prepended with stuff that breaks the flow, although granted that's unlikely. IMO if we need that feature setConfig is not a good place for it, but we can discuss this if/when we get there.

snapwich

just some small feedback. if the things i suggested are not possible for whatever reason then it looks good as-is.

snapwich · 2022-06-10T16:33:59Z

src/prebid.js

-    bidder: Object.fromEntries(Object.entries(config.getBidderConfig()).map(([bidder, cfg]) => [bidder, cfg.ortb2]).filter(([_, ortb2]) => ortb2 != null))
-  }
-  return startAuction({bidsBackHandler, timeout: cbTimeout, adUnits, adUnitCodes, labels, auctionId, ortb2Fragments});
+  ready().then(() => {


i feel like this should use the existing startAuction hook below since it already exists and since hooks execute quickly through synchronous callbacks rather than a deferred promise. by wrapping this in a then it subjects all requestBids calls to the performance penalty of using a promise, even those that are not using debugging.

is there any reason why this can't use the startAuction hook?

I'm taking that to mean that we should hook debugging logic only when we need it - i.e. add/remove hooks when it's toggled - because I don't see how requestBids vs startAuction would have any difference in performance.

add/remove hooks when it's toggled

that wasn't what i was suggesting although now that you mention it you probably could do that.

i was referencing the using existing hook (startAuction) because it already exists at almost the same point where you implemented the debugging ready logic, and if startAuction is sufficient (i.e. not too late) might as well use what already exists.

the performance impact i reference is that by implementing the wait logic with promises you defer the start of the auction to the next execution of the micro-task queue. the hook uses synchronous callbacks to continue execution which means if there are no hooks it will execute immediately.

I'm going the add/remove hook route so we don't need to fight over this, but I'll start anyway: if you mean "go synchronous when you can", with patterns that I saw elsewhere like if (timeout === 0) callback() else setTimeout(callback, timeout), I don't think it's a good idea.

My sense is the performance impact of waiting for the queue to clear is significant only when there's a lot of stuff on the queue (expecting the VM scheduler to be blazingly fast compared to interpreted code), and holding on to control is not the right thing to do in those cases, because the browser freezing for a split second - or, god forbid, the "page is not responding" popup - is a lot more noticeable than some delay in rendering.

Not to mention that at some point we're always going to need to wait for the network, so is shaving (I'm guessing) 1-2ms from 200 or so worth it? but I'll admit that I have not attempted to measure this, so I'm curious why you think it is.

i don't think a if (timeout === 0) callback() else setTimeout(callback, timeout) style is a good practice in general but i think it makes perfect sense in our case. the whole concept of header bidding is to hold an auction in the browser before the page loads with the goal of rendering with the page, and anytime we defer execution we work against that goal.

i don't think this is a case of greedily holding onto resources. requestBids was called which means the publisher meant to start the auction and we've been given our time, so to just immediately defer without cause is:

not being responsible with the time we were given

not an easily quantifiable performance hit. as you said it depends on the size of the queue, that could be nothing or it could be everything. considering we execute at page load we're definitely competing with a lot of other resources though.

is not helpful from a UX perspective (browser freezing, etc). the auction involves an asynchronous request to the bidder endpoints which means we're going to be deferring execution whether we want to or not as soon as we make the requests. why give up our execution time slice early when we know we're just going to be required to defer a few lines of code later?

i'm not anti-promises or anything and i've mentioned with others that i'm comfortable using them where they make sense: with asynconous execution. however i'm very against using them in the critical code path in situations that are expected to be behave synchronously.

I can turn your argument around and say "why not give up control now since we'll be forced to do it as soon as we need the network anyway"?

you could and i'd say because then we'd be giving up control twice instead of the required once. it's like stopping 1 exit early to fill up with gas when you know you need to stop at the next city just a few minutes down the road anyways...

Do we care if debugging is performant?

Or is this modifying the normal workflow

no i don't think it matters too much if debugging is performant (although it is nice to emulate the environment as well as possible)

my main concern is that this impacts the non-debugging code path as well.

Just to confirm - are you referring to the original approach, or the last revision as well? (promises now should only come into the picture when debugging is enabled, and I'm using loadExternalScript as well).

original approach. new revision looks good and i think promises in the debugging flow is acceptable.

snapwich · 2022-06-10T16:38:56Z

src/debugging.js

-      return false;
-    }
-    return true;
+function loadScript(url) {


can this use loadExternalScript in src/adloader.js ?

* WIP: move all debugging logic to the debugging module * Dynamic loading of modules * Move all debugging logic to the debugging module * Automatically load debugging module when needed * Build and dynamically load "standalone" version of debugging module * add note to PR_REVIEW * use loadExternalScript; only add debugging hooks when needed

dgirardi added 4 commits February 22, 2022 12:42

WIP: move all debugging logic to the debugging module

f8393b2

Dynamic loading of modules

b616d0b

Merge branch 'debugging-in-debugging' into debugging

8760a30

Move all debugging logic to the debugging module

d042bd3

dgirardi force-pushed the debugging branch from e594e55 to cc8d559 Compare February 23, 2022 14:38

dgirardi added feature needs review labels Feb 23, 2022

Automatically load debugging module when needed

1db2ec3

dgirardi force-pushed the debugging branch from cc8d559 to 1db2ec3 Compare February 23, 2022 14:54

ChrisHuie requested a review from Rothalack February 24, 2022 12:28

ChrisHuie assigned Rothalack Feb 24, 2022

ChrisHuie requested a review from robertrmartinez February 24, 2022 12:28

ChrisHuie assigned robertrmartinez Feb 24, 2022

dgirardi marked this pull request as draft February 24, 2022 17:03

Build and dynamically load "standalone" version of debugging module

8d1e1ca

dgirardi marked this pull request as ready for review February 24, 2022 20:59

Merge branch 'master' into debugging

1479b88

ChrisHuie requested a review from ncolletti March 31, 2022 18:04

ChrisHuie assigned ncolletti Mar 31, 2022

robertrmartinez removed their request for review March 31, 2022 20:21

robertrmartinez removed their assignment Mar 31, 2022

ncolletti reviewed Apr 21, 2022

View reviewed changes

ChrisHuie self-requested a review April 22, 2022 18:28

patmmccann requested a review from allanjun May 11, 2022 12:24

snapwich self-assigned this Jun 8, 2022

dgirardi added 2 commits June 8, 2022 09:56

Merge branch 'master' into debugging

88ed886

add note to PR_REVIEW

8d243d8

snapwich requested changes Jun 10, 2022

View reviewed changes

use loadExternalScript; only add debugging hooks when needed

55018a7

snapwich approved these changes Jun 11, 2022

View reviewed changes

patmmccann merged commit da46bf2 into prebid:master Jun 14, 2022

dgirardi mentioned this pull request Jun 29, 2022

Core & multiple modules: refactor usage of Promise to avoid deferrals when possible #8626

Merged

1 task

dgirardi mentioned this pull request Nov 3, 2022

Prebid core: fix exception in requestBids introduced by #9106 #9194

Merged

1 task

dgirardi mentioned this pull request Jun 7, 2023

Interest on Yielding on the Main Thread to optimize UX of sites using prebid.js? #10062

Open

dgirardi mentioned this pull request Sep 20, 2023

Dynamically load log messages #10509

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prebid core: automatic dynamic loading of debugging module #8106

Prebid core: automatic dynamic loading of debugging module #8106

dgirardi commented Feb 23, 2022 •

edited

Loading

Rothalack commented Mar 31, 2022

robertrmartinez commented Mar 31, 2022

ncolletti left a comment

ncolletti Apr 21, 2022

patmmccann commented May 11, 2022

allanjun commented May 11, 2022

dgirardi commented May 11, 2022 •

edited

Loading

allanjun commented May 12, 2022

dgirardi commented May 12, 2022

snapwich left a comment

snapwich Jun 10, 2022

dgirardi Jun 10, 2022

snapwich Jun 10, 2022

dgirardi Jun 10, 2022 •

edited

Loading

snapwich Jun 11, 2022

snapwich Jun 11, 2022

patmmccann Jun 11, 2022 •

edited

Loading

snapwich Jun 11, 2022

dgirardi Jun 11, 2022

snapwich Jun 11, 2022

snapwich Jun 10, 2022

		Because of this, this module cannot freely import symbols from core: anything that depends on Prebid global state (which includes, but is not limited to, `config`, `auctionManager`, `adapterManager`, etc) would not work as expected.

		Imports must be limited to logic that is stateless and free of side effects; symbols from `utils.js` are mostly OK, with the notable exception of logging functions (which have a dependency on `config`).

Prebid core: automatic dynamic loading of debugging module #8106

Prebid core: automatic dynamic loading of debugging module #8106

Conversation

dgirardi commented Feb 23, 2022 • edited Loading

Type of change

Description of change

Rothalack commented Mar 31, 2022

robertrmartinez commented Mar 31, 2022

ncolletti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patmmccann commented May 11, 2022

allanjun commented May 11, 2022

dgirardi commented May 11, 2022 • edited Loading

allanjun commented May 12, 2022

dgirardi commented May 12, 2022

snapwich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patmmccann Jun 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgirardi commented Feb 23, 2022 •

edited

Loading

dgirardi commented May 11, 2022 •

edited

Loading

dgirardi Jun 10, 2022 •

edited

Loading

patmmccann Jun 11, 2022 •

edited

Loading