changing default currency file #3414

bretg · 2019-01-02T22:10:39Z

Type of change

[ X ] Does this change affect user-facing APIs or examples documented on http://prebid.org?
[ X ] Other

Description of change

Changing the default currency file location to https://cdn.jsdelivr.net/gh/prebid/currency-file@1/latest.json

Other information

Docs PR at prebid/prebid.github.io#1076

bretg · 2019-01-02T22:12:20Z

Also improved the handling of defaultRates -- @idettman has offered to add a unit test

The currency ajax call doesn't have a timeout. The only events that cause the bidResponseQueue (i.e. bidsToBeConverted) to be processed are:

Successful currency file AJAX response before timeout
Bid response after currency file AJAX response (either successful or error)

I'm assuming that the retrieval of the currency conversion file is going to be fast because it's a small flat file on a CDN, so I think this behavior is ok. i.e. A huge percentage of the time the file should be loaded before the first bidResponse.

bretg · 2019-01-02T22:22:55Z

@mkendall07 - note that this PR changes the behavior of the currency module slightly -- if a bid response comes back before the rates file, formerly it would put the bid on a queue and only process it once the rates file came back.

It's arguable that this is a better approach:

the rates file should be much faster than bids since it's small and on a CDN. (we don't have data on this though)
if the rates file never comes back, at least the defaultRates will be used. Currently they're ignored, which isn't what the docs say happens.

The only scenario that's not as good is 3) when the rates file comes back after the first bid, only subsequent bids will use the more accurate rates -- those bids that came back before the rates file will keep the defaultRates.

Fixing this to support the best of both worlds would require a somewhat larger effort, which we can schedule as a subsequent enhancement?

mkendall07

I'm good with this approach.

snapwich · 2019-01-03T16:31:10Z

@mkendall07 have we confirmed with jsdelivr that they are willing to accept this traffic load? It's probably going to be quite a bit of traffic and is outside their normal use case of serving libraries.

mkendall07 · 2019-01-03T17:04:36Z

Bret did ask about the traffic being an issue and it didn't seem like it was. With that said, if the "rank" here indicates the overall highest traffic for jsdelivr, we'll probably very easily become the top requested resource (https://data.jsdelivr.com/v1/package/npm/jquery/stats)

snapwich · 2019-01-03T17:32:17Z

That's what I'm worried about. We're definitely going to be a noticeable increase to their traffic, so unless we have very explicit permission for what we're doing we'll most likely get removed quickly. Getting removed could be a big issue since we're hard-coding this endpoint into the currency module. If we have explicit permission though, then great.

idettman

LGTM

mkendall07 · 2019-01-03T18:12:22Z

@bretg
Might be worth double checking with jsdelivr on the number of requests for this file

idettman · 2019-01-03T18:48:18Z

Holding off merging until we double check with jsdelivr supporting our expected file traffic rates for the currency file

bretg · 2019-01-03T22:27:18Z

I confirmed with jsdelivr that our expected traffic levels will not be a problem. Removing the hold.

snapwich · 2019-01-04T17:49:36Z

modules/currency.js

@@ -5,7 +5,7 @@ import * as utils from 'src/utils';
 import { config } from 'src/config';
 import { hooks } from 'src/hook.js';

-const DEFAULT_CURRENCY_RATE_URL = 'https://currency.prebid.org/latest.json';
+const DEFAULT_CURRENCY_RATE_URL = 'https://cdn.jsdelivr.net/gh/prebid/currency-file@1/latest.json';


How are we updating this file? In npm it's impossible to republish a given library + version, does jsdelivr not have this restriction, and if not, are we sure they won't have this restriction in the future?

Also, do we have any control over the cache control headers? Currently jsdelivr seems to be setting it to a week, which isn't very good for a currency conversion file.

Source of truth is github, with a new git tag pushed daily via a automated job. We also have access to purge the cache on jsdelivr servers so the cache header shouldn't be an issue. You can see the repo here: https://github.com/prebid/currency-file/releases

I'm a bit worried about the caching here.

First, we need to confirm what kind of caching jsdelivr is doing, currently I can see they're returning the response from a server cache x-served-by: cache-ams21051-AMS, but we don't know the frequency with which it checks the origin for changes; and considering the assumed immutability of a library + version, I wouldn't be surprised if the propagation between a change on github and jsdelivr is quite slow (if it happens at all). Have you guys tested? (just re-read that you said we're invaliding server cache, so that might be sufficient for this problem)

Second, without access to cache control headers it might not be sufficient to just invalidate the cache. Currently on AWS we use expires to tell browsers when the resources is expected to change. Jsdelivr is using cache-control: public, max-age=604800, s-maxage=43200. Without specifying no-cache in the cache-control browsers will not revalidate the etag, which means the browser will serve directly from the resource cache (without checking http headers, which is how it would know that the cache was invalidated).

Good observations @snapwich, but we don't have control over cache control headers with jsdelivr.

Unclear that purging the CDN cache daily will help if the browser's allowed to cache it for a week. I did not do a full analysis of all CDN options, though did ask the community for options to which no one responded.

I will do some quick pricing homework to see if there other options. If not, then 1-week caching is what we get.

it's impossible to republish a given library + version

We set the version of each new file to 1.SEQUENCE+1, and jsdeliver has the '@1' shortcut to take the latest version of 1.x.

Ok - the guy at jsdelivr wants to know what headers we want to set. I think it's just:

Cache-Control: max-age=86400

@snapwich, @mkendall07 - agree?

Mozilla.org (https://developer.mozilla.org/en-US/docs/Web/HTTP/Caching) suggest that Cache-Control: max-age is preferred over Expires.

Can they add expires ?

My vote would be to settle 1 week cache period. This would encourage users to self host if they want more accuracy. The query string would be effective but it's an ugly hack IMO.

I don't mind the cache buster, although I've heard mixed results about query string cache busters (vs filename cache busting).

He said they're planning on adding cache controls in the future including Expires, but did not have a commit date.

Agree that query string is a hack. But it's a hack that preserves existing functionality and saves quite a lot of money, so my vote would be we start off with it as a low-cost workaround to not change the file's loading behavior -- then remove when unnecessary. Am prepared to be outvoted.

The promise of future support for cache and/or expires is good enough for me. @bretg can you make the query string change then?

Done - @idettman has offered to make a unit test.

…ll fix to date macro in currency.js

idettman · 2019-01-09T06:37:10Z

unit tests are ready for review.

mkendall07

LGTM. Thanks for the update!

mkendall07 · 2019-01-09T14:33:02Z

test/spec/modules/currency_spec.js

@@ -30,6 +32,16 @@ describe('currency', function () {
 });

 describe('setConfig', function () {
+ beforeEach(function() {
+ sandbox = sinon.sandbox.create();
+ clock = sinon.useFakeTimers(1047010195974);


nice usage of sinon!

mkendall07 · 2019-01-09T14:33:26Z

modules/currency.js

+ const todaysDate = [d.getFullYear(), month, day].join('');
+
+ // replace $$TODAY$$ with todaysDate
+ url = url.substring(0, macroLocation) + todaysDate + url.substring(macroLocation + 9, url.length);


this is a nit but we usually prefer string literals now.

I committed an update with all the concatenations changed to string literals.

* changing default currency file * using defaultRate in the case of rate file not returning * Adding date macro * added unit tests for date macro support in currency file url, and small fix to date macro in currency.js * replaced string concatenations with string literals * removed unnecessary parens

bretg added 2 commits January 2, 2019 16:13

changing default currency file

4dfc684

using defaultRate in the case of rate file not returning

d8df9e0

bretg assigned idettman Jan 2, 2019

mkendall07 approved these changes Jan 3, 2019

View reviewed changes

mkendall07 added LGTM needs 2nd review Core module updates require two approvals from the core team labels Jan 3, 2019

mkendall07 requested review from snapwich and idettman January 3, 2019 12:50

idettman approved these changes Jan 3, 2019

View reviewed changes

idettman added the on hold label Jan 3, 2019

harpere removed the needs 2nd review Core module updates require two approvals from the core team label Jan 3, 2019

bretg removed the on hold label Jan 3, 2019

snapwich requested changes Jan 4, 2019

View reviewed changes

harpere added the on hold label Jan 4, 2019

bretg and others added 2 commits January 7, 2019 18:51

Adding date macro

0fd09ea

added unit tests for date macro support in currency file url, and sma…

1956909

…ll fix to date macro in currency.js

mkendall07 approved these changes Jan 9, 2019

View reviewed changes

idettman added 2 commits January 9, 2019 08:49

replaced string concatenations with string literals

7b32efe

removed unnecessary parens

3147473

mkendall07 merged commit 4b53954 into master Jan 9, 2019

benjaminch mentioned this pull request Jan 10, 2019

[Currency] Changing default currency file URL prebid/prebid-server#785

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changing default currency file #3414

changing default currency file #3414

bretg commented Jan 2, 2019

bretg commented Jan 2, 2019

bretg commented Jan 2, 2019

mkendall07 left a comment

snapwich commented Jan 3, 2019

mkendall07 commented Jan 3, 2019

snapwich commented Jan 3, 2019

idettman left a comment

mkendall07 commented Jan 3, 2019

idettman commented Jan 3, 2019

bretg commented Jan 3, 2019

snapwich Jan 4, 2019

mkendall07 Jan 4, 2019

snapwich Jan 4, 2019 •

edited

Loading

bretg Jan 4, 2019

bretg Jan 4, 2019

mkendall07 Jan 7, 2019

snapwich Jan 7, 2019

bretg Jan 7, 2019

mkendall07 Jan 7, 2019

bretg Jan 8, 2019

idettman commented Jan 9, 2019

mkendall07 left a comment

mkendall07 Jan 9, 2019

mkendall07 Jan 9, 2019

idettman Jan 9, 2019

changing default currency file #3414

changing default currency file #3414

Conversation

bretg commented Jan 2, 2019

Type of change

Description of change

Other information

bretg commented Jan 2, 2019

bretg commented Jan 2, 2019

mkendall07 left a comment

Choose a reason for hiding this comment

snapwich commented Jan 3, 2019

mkendall07 commented Jan 3, 2019

snapwich commented Jan 3, 2019

idettman left a comment

Choose a reason for hiding this comment

mkendall07 commented Jan 3, 2019

idettman commented Jan 3, 2019

bretg commented Jan 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snapwich Jan 4, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idettman commented Jan 9, 2019

mkendall07 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

snapwich Jan 4, 2019 •

edited

Loading