Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move the initPermutive call #1782

Merged
merged 7 commits into from
Feb 19, 2025
Merged

Move the initPermutive call #1782

merged 7 commits into from
Feb 19, 2025

Conversation

emma-imber
Copy link
Contributor

@emma-imber emma-imber commented Feb 5, 2025

What does this change?

Moves the initPermutive() call from consented-advertising to be a dependency of fillStaticAdvertSlots, behind a 0% test. I've added a test offset of 20% to avoid the changes in this PR

Why?

At the moment, initPermutive() has to happen before prepareGoogletag, so it blocks the loading of the googletag script. By moving it to be a dependency of fillStaticAdvertSlots, we can load the googletag script a little earlier.

The documentation states that the initPermutive() code must run before googletag.enableServices() is called. googletag.enableServices() is called by displayAds and displayLazyAds, both of which are only called by fillStaticAdvertSlots, so as long as initPermutive runs before that, we should be all good.

@emma-imber emma-imber requested a review from a team as a code owner February 5, 2025 13:09
Copy link

changeset-bot bot commented Feb 5, 2025

🦋 Changeset detected

Latest commit: 83f9875

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@guardian/commercial Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Feb 5, 2025

Ad load time test results

For consented, top-above-nav took on average 4434ms to load.
For consentless, top-above-nav took on average 3142ms to load.

Test conditions:

  • 5mbps download speed
  • 1.5mbps upload speed
  • 150ms latency

Copy link
Contributor

@dskamiotis dskamiotis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sensible to me, especially made clearer given the refactoring work you did in the PR: #1764

Would be interesting to see the metrics we can use to test (assuming in CODE as you mentioned?)

@emma-imber emma-imber force-pushed the ei/move-initPermutive branch from edf7ad3 to c614118 Compare February 12, 2025 14:30
start: '2025-01-14',
expiry: '2025-02-28',
audience: 0 / 100,
audienceOffset: 20 / 100,
Copy link
Contributor

@dskamiotis dskamiotis Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know its a 0% test, Wondered what this does audienceOffset: 20 / 100

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The offset is to make sure that if we set it live (eg not 0%) that the test audience wouldn't overlap with that of your test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see great, Ill keep mine to 0/100 👍

@emma-imber emma-imber merged commit 6f4fe3a into main Feb 19, 2025
14 checks passed
@emma-imber emma-imber deleted the ei/move-initPermutive branch February 19, 2025 15:32
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @emma-imber 1 minute and 40 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants