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

No longer proxy RTD ads through RTD servers #7506

Merged
merged 7 commits into from Oct 19, 2020

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Sep 23, 2020

  • Instead of proxying ads through readthedocs.org/api/..., hit EthicalAds directly. This has a lot of performance advantages from a server scalability perspective since there won't be any process on RTD servers waiting on ad responses.
  • This relies on additional data (whether ad free, community ads only, keywords) being sent in the Footer API response or in having a new API endpoint.
  • Rely on viewport detection in the ad client (Add viewport detection using the Verge module ethical-ad-client#29) which will be present in the beta version
  • Removes the footer ad type and the fixed footer ad types entirely. Instead, we will place a regular ad in the footer if the sidebar would push the ad off the screen.
  • From a timing perspective, when the DOM is ready, we immediately query the new RTD user data endpoint and concurrently load the ethical ad client. When the RTD user data endpoint returns, that's when we request an ad. The performance of this shouldn't be significantly worse than our current code.

Open Questions/To-Do

  • We might bring back the sustainability API but instead of proxying the connection to the ad server, it just returns the data necessary to display an ad. (Edit: we did this)
    • To get the functionality on par with our existing system, we also want to send keywords related to the project to the ad server. This is used for content targeting.
    • We have a few projects on RTD where revenue is shared. These have specific publisher IDs which we will also need to sent to the ad server.
  • The ad client only works for image or text ads. Any other custom types might need a modification. The ad client appends v1 to either the image or text ad type. (Edit: added in Add keywords and campaign types to the client ethical-ad-client#31)
  • There's some additional functionality I stripped out in the initial implementation to pick other ad types (footer ads, mostly). This will need to come back with some modifications to support the ad client. (Edit: these features are back)

Screenshot

Screen Shot 2020-09-23 at 3 41 23 PM

- Instead, hit EthicalAds directly
- Relies on additional data sent through the footer API
@davidfischer davidfischer added the PR: work in progress Pull request is not ready for full review label Sep 23, 2020
@davidfischer davidfischer requested a review from ericholscher Sep 23, 2020
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This seems like a good start. I see the issues arounds special casing "publishers within a publisher" and some of the other integration data we were doing on the server side. I don't have a great solution tho, but I think it'll be useful to figure out if we can.

def is_ad_free_user(self, user):
if not settings.USE_PROMOS:
return True
if user.is_authenticated and hasattr(user, 'gold') and hasattr(user, 'goldonce') and (user.gold.exists() or user.goldonce.exists()):
Copy link
Member

Choose a reason for hiding this comment

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

I think we want goldonce or gold uses right, not and?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the logic is correct. It is an OR except first checking that the attributes exist. Since this API can be called even when the gold app or the donate app are not installed, this extra logic is necessary here.

Copy link
Member

@humitos humitos Sep 24, 2020

Choose a reason for hiding this comment

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

I think Eric is right.

In the case that hasattr(user, 'gold') -> True and user.gold.exists() -> True, but hasattr(user, 'goldonce') -> False it won't enter the IF block but it seems that it's currently ad free.

However, I'm not sure if it's possible that gold exists but not goldonce, tho.

Maybe this change?

if user.is_authenticated and ((hasattr(user, 'gold') and user.gold.exists()) or (hasattr(user, 'goldonce') and user.goldonce.exists())):

def is_ad_free_project(self, project):
if not settings.USE_PROMOS:
return True
if project and hasattr(project, 'gold_owners') and (project.gold_owners.exists() or project.ad_free):
Copy link
Member

Choose a reason for hiding this comment

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

Won't this also make ad_free projects not work if they don't have gold_owners?

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 check hasattr(project, 'gold_owners') just checks whether that object is present, not whether there actually is a gold owner. This is needed because the gold app may not be installed.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha -- we should probably just check for that explicitly (if 'donate' in settings.INSTALLED_APPS), or at least add a comment. It isn't clear that's the reasoning from the code.

Copy link
Member

Choose a reason for hiding this comment

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

When this if's gets complicated on the or things, I try to split per each case to avoid confusions, like:

if not settings.USE_PROMOS:
  # commercial site
  return True

if project.ad_free:
  # marked manually as ad free
  return True

if project and hasattr(project, 'gold_owners') and project.gold_owners.exists():
  # project's owner is gold member
  return True

return False

You end up with multiple exits where the output is the same, True, but it's clearer to read, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing, we're going to check settings.INSTALLED_APPS.

@@ -78,6 +79,10 @@ function init() {
}
injectFooter(data);
setupBookmarkCSRFToken();

if (!data.ad_free_user && !data.ad_free_project) {
sponsorship.init();
Copy link
Member

Choose a reason for hiding this comment

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

I imagine this is going to add a decent bit of latency to ad viewing, I wonder if we should do the ads request always, but vary the display based on this data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will add some latency, but I'm not sure how you can actually do what you're saying. Either you add the ethicalads.js meaning you want an ad or you don't. To determine that, you need to know some additional data about the user or project. Having a stripped down API that just gets the project/user data would likely be faster although there would be an additional concurrent API call.

Copy link
Member

Choose a reason for hiding this comment

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

I think we do the ad request, but we can hide it with CSS until we confirm we want to show it. That way as soon as the footer responds, we can display the ad, instead of doing another full round trip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we should be hiding ads with CSS and showing them based on API responses. I think this will lead to complication. Instead, perhaps there's a way to add some integration points to the ad client so we can control when requests for an ad are made.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can start with this approach for now. But I do think trying to reduce latency on the ad display is important. The only way to reduce latency is to do the ad load on initial page load, and then display it later based on data from the server. If we don't do that, it doesn't matter what approach we take, it will be quite slow.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be hiding ads with CSS and showing them based on API responses

Is it possible to do the request for the ad, save all the data needed (view URL, click URL, image URL, text, etc) without creating the HTML element to display it yet, until we receive the response from the footer and there create the HTML element and show it?

I assume the ad-client is not thought to work like that at this point, but I think we could have the best of both ideas with lower latency.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, this is what we settled on 👍

};
container = $('<div />').attr('style', 'text-align:center').appendTo(selector);
$('<div />')
.attr('data-ea-publisher', "readthedocs")
Copy link
Member

Choose a reason for hiding this comment

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

We could include the publisher group in the footer response to adjust this for our revshare folks, but that does require blocking this on the server response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think you can initialize ads without first getting the project/user data. All the decisions you want to make require that data.

Copy link
Member

Choose a reason for hiding this comment

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

One idea: 99% of our pageviews aren't revshare or gold users, so I still think loading the ad JS makes sense at pageload, but we can hide the ad display until we get the footer data back. That will keep ads at the same latency as now, and we just need to re-request them on revshare projects. We could even dump some metadata into the built HTML for revshare projects to be able to read that data prior to the initial request.

This seems like it'll get us almost every ad view in the same latency as the previous approach, if not faster.

@ericholscher
Copy link
Member

We talked about this on a call, and the proposed path forward:

  • Have revshare users add the EA div to their docs directly, so we know the publisher
  • Update the ad client so it can fetch ads, but not display them
  • Add a lightweight ads api that just returns gold user status
  • Request the ad & footer at pageload, then only display the ad once we confirm it isn't a gold user.

@davidfischer
Copy link
Contributor Author

This has been updated and I updated the description. It is ready for a review.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good 🎉

Looks like eslint is failing, so just need to fix that up.

@davidfischer
Copy link
Contributor Author

This is testing out really well. Here's how you can test it:

  • Set USE_PROMOS = True in readthedocs/settings/docker_compose.py
  • Build the latest static assets on RTD npm run build
  • Load ads on a built project. Verify that the view (https://server.ethicalads.io/proxy/view/...) is triggered only if the ad is in the viewport. The ad should be loaded after the gold user/project is checked.

@ericholscher
Copy link
Member

Tested this locally and it looks 💯

@davidfischer davidfischer merged commit c91da7f into master Oct 19, 2020
2 checks passed
@davidfischer davidfischer deleted the davidfischer/stop-proxying-ads branch Oct 19, 2020
@davidfischer davidfischer removed the PR: work in progress Pull request is not ready for full review label Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants