prebid / Prebid.js

Setup and manage header bidding advertising partners without writing code or confusing line items. Prebid.js is open source and free.
https://docs.prebid.org
Apache License 2.0
1.28k stars 2.06k forks source link

Improve client-side footprint for server-side adapters #6361

Closed bretg closed 4 months ago

bretg commented 3 years ago

Type of issue

enhancement

This issue has been updated significantly based on progress over the past 8 months. It used to be a high priority, but something has been fixed in the meantime, and now it's no problem to run a server-side bidder without client-side code.

Description

One of the most common complaints about Prebid.js is that the resulting javascript package size is too big.

Moving code server-side should be a way to reduce the package size, but there are a couple of bidder-specific elements in the client-side BidAdapter spec that might be useful even when the bidder runs server-side:

So of these only the first, transformBidParams, is really compelling nowadays. Procrastination does sometimes pay off.

jsnellbaker commented 3 years ago

Just to highlight it, there is also a transformBidParams function used by (roughly) a dozen bid adapters to update the formatting used by the bid params to make them compliant for PBS.

Below are some links to where this is being used (for reference): https://github.com/prebid/Prebid.js/blob/master/modules/prebidServerBidAdapter/index.js#L613 https://github.com/prebid/Prebid.js/blob/master/modules/appnexusBidAdapter.js#L338

muuki88 commented 3 years ago

@bretg there is something in the web as well :smile: I saw it on the the prebid-outstream project: webpack-conditional-loader.

This would at least work for NPM modules, but requires all adapters to include these special comments for this loader. And it would only work for those that build prebid with webpack themselves.

muuki88 commented 3 years ago

I have another suggestion for this issue, but I'm not sure if this could work.

The idea is separate bid adapter, e.g. pbsStoredImpressionAdapter, that is configured with a stored impression id and some other meta data. In the end this adapter sends a regular openRTB request along with the impression id. The GVL_ID could be configurable.

A usage may look like this

const bid = {
  bidder: 'pbs',
  params: {
      // required
      storedImpressionId: 'position_1',
      // optional
      bidCurrency: 'EUR',
      bidFloor: 0.10,
      targeting: {
      }
  }
} 

WDYT?

snapwich commented 3 years ago

So I dug into this a bit, I think the best approach would be a build-time flag and using the dead-code removal already provided by webpack. I did a little proof of concept and verified it would be a simple addition to the webpack config.

using the DefinePlugin:

var plugins = [
  new RequireEnsureWithoutJsonp(),
  new webpack.EnvironmentPlugin(['LiveConnectMode']),
  new webpack.DefinePlugin({
    __LITE__: JSON.stringify(!!argv.lite) // set global __LITE__ based on --lite cli flag
  })
];

this allows for different code to be included in the build depending on the ---lite flag being present: e.g. in an adpater

import { registerBidder } from '../../src/adapters/bidderFactory.js';

// ... shared code

if (__LITE__) {
  registerBidder({
    // ... minimal adapter code
  });
} else {
  registerBidder({
    // ... heavy adapter code
  })
}

A few reasons I prefer this approach over something like the webpack-conditional-loader.

  1. doesn't require a third-party dependency since webpack/uglify do dead-code removal already
  2. only removes code in the production version, the dev version still contains both code paths which could allow for debugging features (such as toggling between versions during development)
  3. since it uses regular javascript syntax tooling will work correctly, e.g. IDEs, linters, sourcemaps. using custom macros could cause issues such as code that by static analysis appears valid but breaks during the build process.
  4. also since it's javascript syntax it allows for fine-grained code removal within expressions, e.g. using ternary operators (?), or boolean operators (&&, ||) (tested and verified code like {blah: __LITE__ && ... some small expression} will successfully be removed mid-expression if flag is enabled due to javascripts short-circuiting logic)

This could be used in adapters or core to produce separate builds based on different requirements. One thing it doesn't allow for is completely removing import statements (as imports cannot be within conditions). I'm not sure that's something we really want though? The imports within adapters already doesn't result in additional code for an adapter as the webpack bundle optimizer de-dupes those into the prebid-core anyways.

I still think a completely separate version of prebid.js relying on open standards could be an additional approach we look at (e.g. https://github.com/prebid/Prebid.js/issues/5001), but this could be a useful midway approach.

patmmccann commented 3 years ago

@snapwich what do you think of @pm-harshad-mane proposal to use gulp plugin instead? https://github.com/prebid/Prebid.js/pull/6079

If 6079 is merged, it seems that is extremely analogous functionality.

Also i think we may not want to call s2s only 'lite', but more explicitly 's2sonly' as lite could be banner only or something else diet.

Notably, there is already a liveconnect lite module that removes some build time dependency using gulp instead of webpack https://github.com/prebid/Prebid.js/pull/6016

bretg commented 3 years ago

FYI - I added getUserSyncs to the list of spec entries needed by server-side adapters. client-side user syncs are still a thing for another 9 months even for server-side bidders.

snapwich commented 3 years ago

@patmmccann as i stated i'm not a huge fan of the comment macros to remove sections of code. i'd prefer using dead-code removal as this allows us to use regular javascript syntax that allows linters and static analysis to work correctly on our codebase.

e.g.

// removeIf(feature)
if (something) {
  ...
// endRemoveIf(feature)
}

that code is broken but not according to regular static analysis and javascript linters.

also plugins like that would break sourcemaps: https://github.com/crissdev/gulp-remove-code/issues/8

using webpack's built-in dead-code removal is guaranteed to work with the rest of webpack's features such as source map creation.

i also think we don't want to allow for feature flags in general, doing that creates a testing nightmare. i'd propose a regular prebid.js and a s2s version of prebid.js.

bretg commented 3 years ago

@snapwich - I'm not following how dead code elimination helps here

propose a regular prebid.js and a s2s version of prebid.js

A "PBJS lite" might happen eventually, but for now, this issue is about pubs that want to push a subset of bidders server-side. Currently they have to include the full client-side adapter even if they want to run server-side. Which fails to confer any size savings.

It would be pretty burdensome to ask all 100+ adapters to build and maintain server-only modules.

Any other approaches?

mike-chowla commented 3 years ago

I agree with @bretg that maintaining separate server-only modules will be hassle and that the current need is for a mix of client-side and s2s bidding, controllable on an adapter-by-adapter basis. Being able to keep the functions that are needed for s2s and drop the other ones only needed for client-side bidding sounds like the ideal approach to me. I'd also like to be able to drop format specific code from adapters at build time when it's not needed.

For the adapter s2S code reduction, the build process would need to take a list of adapters that should be s2s only. My knowledge of JavaScipt builds is no where good enough to figure how this would be implemented.

muuki88 commented 3 years ago

At the moment we are testing the following setup

  1. One client-side adapter that runs only on server side
  2. Configure a stored impression id for every ad unit (#6494)
  3. Add workaround described in #7027
  4. Configure the stored impressions on server side only

This results in no client adapter code being added and it works.

What I have not yet fully understood what features a relevant in prebid.js when only using server-side. The AMP endpoint from prebid server returns a targetings object, which is fed to the ad server. For web there is quite a lot more to do.

@bretg do you think it makes sense which of the modules could be moved to server side and which have to live on the client?

bretg commented 3 years ago

do you think it makes sense which of the modules could be moved to server side and which have to live on the client?

I would say that Prebid will someday get serious about "PBS-Lite" and at that point we'll prioritize which of the many modules need to get ported to the server side. Some functions (e.g. currency) already are. Others (e.g. floors) are planned. But many are vendor-related (e.g. RTD), and we're likely to have a bit of a chicken-egg problem .

I'm still of the opinion that comment macros are the way to go here.

Server-side adapters still require the following client-side elements:

1) MediaTypes 2) Aliases 3) GVLID 4) transformBidParams function

I'll take it a step further and propose a specific straw syntax:

gulp build --modules=bidderABidAdapter,bidderBBidAdapter,bidderCBidAdapter --globalmacros=NO_NATIVE,NO_VIDEO --s2sOnly=bidderABidAdapter,bidderBBidAdapter

Beat it up, make it better! We need to act on this issue.

muuki88 commented 2 years ago

At some point I'll try to prototype a minimal "Prebid.lite" or "Prebid.less" thing that has the same API surface as Prebid, but in the end only sends s2s requests. My gut feeling is that there are tons of utility features in prebid core that won't be necessary if things are handled on the server-side.

I agree with id-modules, RTD and FPD modules still being present on the client-side, but something like transformBids maybe still be possible to move to server-side. Regarding aliases and GVLID, this should already be present on the server-side, right?

All my gut feelings come from the AMP implementation, which ultimately returns a targeting object that's put on each ad slot. From my point of view this is sufficient enough. The requests differ though as we can add more information (user, context, etc.) that is only present client side.

Not sure if magic comments (or macros) are the way :smile: The workaround I described above works pretty well, even it generates some noise in our analytics data. We only add the bidders which provide an transformBidParams function (e.g. SmartAdServer). So my point is to use this opportunity to setup prebid from the ground up.

Ususally rewrites should be avoided as all the knowledge gained over the years are lost, but my argument would be that 80% of prebid.js don't need to be ported (e.g. adapters, a lot of the auction logic, currency)

bretg commented 2 years ago

I think our goal here should be to get rid of the client-side footprint altogether.

Did an analysis of how transformBidParams is used

So I think we could drop transformBidParams entirely if we:

  1. Supported server-side parameter type conversion. I think this is already supported in a basic way, at least in PBS-Java.
  2. Added one or more modules that enhanced the requests with params that match what Adagio's doing. i.e. set standard conventions for slot position, etc.
muuki88 commented 2 years ago

I like that approach. How would you imagine the gvlid handling? E.g. if the gdprEnforcement module is enabled and a bid adapter is missing, then the bidder will be blocked due to the lack of a gvlid. Of course this can be solved via vendor exceptions, but it feels a little clunky :smile:

bretg commented 2 years ago

If we want to keep the javascript at a minimum, shouldn't we get rid of client-side enforcement altogether? Here are the modules that would not be needed in a prebid.less scenario IMO:

As for GVL ID, I would suggest that either PBS needs to be able to recognize different GVLIDs for aliases (PBS-Java does already) or the pub could just specify it in on the page. https://docs.prebid.org/dev-docs/publisher-api-reference/aliasBidder.html

muuki88 commented 2 years ago

That's a fantastic list :heart: You selected the items based on the assumption that there are no client adapters at all, right? Otherwise I would argue that at least

And the others are optional - only if a client adapter requires it publishers can add it.

bretg commented 2 years ago

Right Muki - I was thinking of the prebid.less scenario, but we're a ways off from that. Baby steps. All of these modules are needed as long as there's client-side bid adapters.

slayser8 commented 2 years ago

Is this something we can prioritize in PbS? Happy to put some engineers behind it if need be but would be great to collaborate...

bretg commented 2 years ago

There's not much Prebid Server work to do @slayser8 . I think all that's needed is to coordinate amongst the adapters that currently use the transformBidParams() function. I can reach out to the list of bidders noted above and get them started on this.

So as far as not needing a client-side adapter, I really think we're mostly there, right? I just tested this locally again, and there's no problem making a rubicon server-side adapter call without including the rubicon client-side adapter. Except that the rubicon server-side adapter will only accept integers as parameters. The transformBidParams() function is just a robustness enhancement.

The only other thing that seems to be a work item is a Prebid.js item driven by Adagio -- they want some parameters that the pbsBidAdapter isn't currently setting. I'm willing to work with them (hello @osazos !) to work through what's needed.

Does everyone agree that we can close this issue when:

bretg commented 2 years ago

Opened the Prebid Server issue to start that conversation.

adRelevantis (@ghguo) and craft (@crumbjp) - you guys don't seem to have Prebid Server adapters. Why do your client-side adapters define transformBidParams?

@osazos - Adagio's bid adapter has the most sophisticated transformBidParams function. Here's my proposal:

osazos commented 2 years ago

Hi @bretg Sorry for my late response I just create a PR to remove our prebid-server adapter and I have to update our Prebid.js adapter to remove the stuff we've done to be compliant with Prebid-server.

Regarding your proposal, thanks a lot to have taken time on this! So as we remove our prebid-server adapter, there is some points which are obsolete. But if some can be added to Prebid.js itself, it is nice and I can contribute with it.

adagioBid.params.features.print_number: we can add this in the pbsBidAdapter as imp.ext.data.prebid.auctioncount

I have to dig about how we get our "print_number". Actually we get the value of bidRequestsCount, but it seems the value can be incorrect. For example, sometimes I get bidRequestsCount = 2 when I see only one request. As I said, I will dig into this issue.

adagioBid.params.features.adunit_position: this one's hard. I propose we take your position-definition logic and make it a module that sets AdUnit.banner.pos or AdUnit.video.pos

It could be nice for Prebid.js, but it should be added by default and usable in adapters if needed. We want to avoid that the adagioBidAdapter depends on some modules to work. This feature is critical for us.

adagioBid.params.pageviewId: I don't think Prebid has this concept, but it could be added. What's the use case?

It is for internal stuff. We use pageviewId on all our chain to match data. This feature is critical for us. Adding this concept to Prebid Core can be a good idea, but we have to keep in mind this data must be get outside of Prebid.js itself.

adagioBid.params.features.page_dimensions: pbsBidAdapter can add this to device.ext.prebid.pagedimensions adagioBid.params.features.viewport_dimensions: pbsBidAdapter can add this to device.ext.prebid.pagedimensions

This is not the same features, the page_dimensions is computed regarding the html document size. The viewport dimensions is the pseudo screen size. It could be nice to add these to Prebid.js core.

adagioBid.params.features.dom_loading: This is obscure. Why do you need this? Can it be dropped?

It is used internally. I can't drop it.

exchangeData: your requirements here aren't clear. Please help us figure out how to supply these server-side.

This is use to pass data between our measurement script, our ssp and Prebid.js. I'm not sure if this one will stay in our codebase.

bretg commented 2 years ago

I have to dig about how we get our "print_number".

Cool - I'd like to collect a list of easy pbsBidAdapter upgrades for prioritization. Then there are at least two hard ones.

Easy:

Hard:

Unknown:

osazos commented 2 years ago

HI @bretg

With a new regard on your last comment, adunit_position and mostly pageviewId/user_timestamp are specific to Adagio and should be ignored in the context of this issue (adunit_position could be part of a module is necessary as you said)

The others ones are easy to implement (even dom_loading) and I think we could add these as utility functions (utils.js). This way we have the choice:

regarding the print_number, this counter is the number of times an adunit has been rendered. Actually Prebid.js counts the number of requests done only. Is this feature could be implemented at the same level of bidRequestsCount/bidderRequestsCount?

patmmccann commented 10 months ago

@muuki88 Can we add a documentation field along the lines of needs client side adapter to run server side because of the transformBidParams . Known examples are Pubmatic and Adagio

Can we also document explicitly an example of calling a server side bidder without installing their client side bidder?

patmmccann commented 10 months ago

@bretg to determine if https://github.com/prebid/Prebid.js/blob/bf909eaa066c997ee169640008ee5933c1965874/modules/rubiconBidAdapter.js#L776 is still necessary

patmmccann commented 10 months ago

https://github.com/prebid/Prebid.js/blob/bf909eaa066c997ee169640008ee5933c1965874/modules/adagioBidAdapter.js#L1288 is a notable update

patmmccann commented 10 months ago

Prebid 9 proposal: drop support for transformBidParams in favor of massive potential client-side build size reduction and server side handling of the conversions.

muuki88 commented 10 months ago

@patmmccann that makes sense. Sorry, I couldn't make it to today's PMC.

We'll discuss the meta field and how this can be implemented in the docs PMC.

I'll create an issue to track this and also the example on how to run this.

bretg commented 9 months ago

Updated the analysis for the adapters that have defined transformBidParams. Here are the scenarios:

Empty Function or no Server-Side Adapter.

These are easy -- just remove the function as unnecessary.

DataType Conversions

All of these adapters just convert parameters to a particular data type to deal with the javascript-vs-server-side type safety issue. In all cases, the server-side adapters can be robust for string/integer comparisons with these steps:

  1. Enhance the parameter JSON schema with multiple datatypes. e.g. https://github.com/prebid/prebid-server-java/blob/8095edadea67f0a3a07ddbc34a37003f0f290377/src/main/resources/static/bidder-params/rubicon.json#L16
  2. Make sure the bid adapter code can handle either string or int.

Unknown, Needs more Work

bretg commented 9 months ago

Confirmed that the Rubicon server-side adapters handle types, so opened PR to remove the transformBidParams function. https://github.com/prebid/Prebid.js/pull/10919

patmmccann commented 5 months ago

@samuel-palmer-relevant-digital fyi

samuel-palmer-relevant-digital commented 5 months ago

Hi @patmmccann etc,

About transformBidParams in relevantdigitalBidAdapter.js. What is happening is that client-side - one have the option to set the accountId and pbsHost settings using pbjs.setConfig() instead of supplying them in params (it can be done either way). Example here

As our server-side bid adapter requires these settings as well, the transformBidParams copies the (potentially) missing settings into params.

But as I'm not even sure if anyone is using our adapter on web with S2S along with the setConfig() option setting, I would say it's not "critical" for us to keep that function.

bretg commented 5 months ago

Thanks for the explanation @samuel-palmer-relevant-digital

not even sure if anyone is using our adapter on web with S2S along with the setConfig() option setting, I would say it's not "critical" for us to keep that function.

Yeah, sounds like it might be an item in the PBJS 9.0 Release Notes

patmmccann commented 5 months ago

@samuel-palmer-relevant-digital we'd appreciate your PR! thanks!

GeneGenie commented 4 months ago

Hi, we have added PR to adress this issue. https://github.com/prebid/prebid-server/pull/3676

patmmccann commented 4 months ago

@GeneGenie did you PR your js adapter?

patmmccann commented 4 months ago

11412 got relevant; https://github.com/prebid/Prebid.js/pull/11585 got the rest