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

Topics Module: Support for Fetch Header Functionality #10124

Merged
merged 7 commits into from
Aug 9, 2023

Conversation

jlquaccia
Copy link
Collaborator

Type of change

  • Feature

Description of change

  • added optional configurable support to integrate with Topics API header/fetch technique to obtain topics
  • added configurable support set the duration (in days) for how often fetch calls should be made
  • created a test/example server endpoint demonstrating how an ssp topics fetch header endpoint could look like (not sure if we would like to keep this, but I left it in for now)

Other information

GitHub Issue Ticket: #9981

@jlquaccia
Copy link
Collaborator Author

After code review/feedback I can update the docs for this PR as well. Also, I had 1 question while working on this ticket. I noticed that when storing topics in local storage, we are currently only storing the first index of whatever came back in the topics array, is this expected? I know at times the topics array can have more than 1 item in it. See the following links for a reference. Came across that and was curious.

const iframeTopicsData = getTopicsData(domain, topics)[0];

const fetchTopicsData = getTopicsData(domain, topics)[0];
(i followed the same pattern with the fetch endpoint here)

@patmmccann
Copy link
Collaborator

when storing topics in local storage, we are currently only storing the first index of whatever came back in the topics array, is this expected?

looks like a bug to me, good catch

@jlquaccia
Copy link
Collaborator Author

Just wanted to follow up on a review for this one. @patmmccann @dgirardi let me know what you guys think, or if any changes should be made.

const topics = config.getConfig('userSync.topics') || bidderIframeList;

if (topics) {
listenMessagesFromTopicIframe();
const randomBidders = getRandomBidders(topics.bidders || [], topics.maxTopicCaller || 1)

Choose a reason for hiding this comment

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

Is it possible to set the default behaviour to all bidders receiving their topic instead of only 1. Limiting to 1 by default would introduce uncertainty and some unfairness for bidders that will rely on the topics header for their bidding logic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@remysaissy i agree with you that we shouldn't default to 1 (I think that was added in there during an initial PR for this, as the logic around Topics is still being figured out..) Thinking that maybe the default for this module should be off? aka no publishers? since this module requires config setup anyway. Or, maybe if there is some sort of prebid general iframe/fetch url to point to as a default? not sure how that would be exactly

Choose a reason for hiding this comment

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

Following @patmmccann's remark on feature request discussion to be in issues, I've followed-up on this on the issue: #9981 (comment)

modules/topicsFpdModule.js Show resolved Hide resolved
modules/topicsFpdModule.js Show resolved Hide resolved
const bidderLsEntry = storedSegments.get(bidder);

if (!bidderLsEntry || (bidderLsEntry && isCachedDataExpired(bidderLsEntry[lastUpdated], fetchRate || DEFAULT_FETCH_RATE_IN_DAYS))) {
window.fetch(`${fetchUrl}?bidder=${bidder}`, {browsingTopics: true})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have some dumb questions about this - due to how poorly I understand topics.

I read this as "get some topics to add to the bid stream, while letting the remote end know the topics we already have in this origin".

I don't understand why the endpoint - which I believe is meant to be an SSP - needs to know the topics we see on our end, if all they're doing is telling us what we should add to the list of topics sent out later in the auction. What are they doing with them?

I'd understand a bit more if this change was about enabling the topics header on requests sent out by bidders - although even there, how is better than populating ORTB, which we are doing already?

Choose a reason for hiding this comment

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

It's maybe because the topics one can see on the page depends on the origin of the caller?

All partners in the same page would not necessarily see the same topics for the same auction (what I mean is that, for the same browser, on the same page, at the same moment, the browser will not respond with the same topics API depending on the origin (eTLD+1) of the call.

So, every bidder has some interest to fetch the topics observed by their own origin domain (eTLD+1).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's maybe because the topics one can see on the page depends on the origin of the caller?

This may make sense for iframes (if the iframes we are doing make sense - aren't they just a way to circumvent this "in-browser only" paradigm?), but the new call in this PR is just hitting a JSON endpoint, there's no page with its own topics that may be affected by this.

Copy link
Collaborator

@patmmccann patmmccann Jul 24, 2023

Choose a reason for hiding this comment

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

@dgirardi please see https://developer.chrome.com/en/docs/privacy-sandbox/topics/#use-headers-to-access-and-observe-topics and https://developer.chrome.com/docs/privacy-sandbox/topics/demo/#the-topics-api-headers-demo

In order for the browser fetch to include the topics observed over the user lifetime, the fetch must be made in this way. The 'last week' topics for the users observable to that endpoint are included, not the topic of the current request. The true or false is actually instructions to the browser on whether or not to use the current referrer as training for 'next week's topics.' The module assumption is that the endpoint will simply reflect that header back to the user as a json.
image

@jlquaccia
Copy link
Collaborator Author

@github-xavier-bucchiotty I see what you mean, and also agree that utilizing the bid request to integrate with the topics fetch header functionality sounds like it makes the most sense.

so would this then mean that:

  • when bid requests go out, prebid could include the {browsingTopics: true} option (sounds like requests would need to be fetch because of the xhr topics header deprecation, as was noted in a comment earlier on this PR)
  • the browsingTopics option would trigger the Sec-Browsing-Topics request header to be appended to the bid request and sent out
  • the ssp can then interpret the value of Sec-Browsing-Topics once the bid request is received on their end (if there is even a value present at all) and act on it

Also, from https://developer.chrome.com/docs/privacy-sandbox/topics/demo/#headers, it sounds like the value for Sec-Browsing-Topics will list topics observed for the request URL hostname that have been observed for the current user's browser? (if that sounds correct?) So like stated earlier, these topic values would of course vary by what a specific user has been viewing in their browser

Below is from the doc page, but want to double check if it sounds like i am interpreting it right..

In browsers that support the API, the fetch() request will include a Sec-Browsing-Topics header that lists topics observed for the request URL hostname: in this example, topics-server.glitch.me.

If no topics have been observed for this hostname and this user, the header is included but the value is empty. In other words, the Sec-Browsing-Topics header on a fetch() request only includes topics that have been observed for the current user's browser by a caller whose origin matches the hostname of the request URL.

@patmmccann
Copy link
Collaborator

@jlquaccia I would encourage you not to take the teads team comments as feedback on your PR, which solves the issue as described, but instead a different feature request altogether, which should move to a different location on github for its own discussion. The PR appears to solve the issue as described, let's base our reviews on that.

@remysaissy
Copy link

It was not our intention to make feature requests in the PR as we left a comment for feature request in the related issue as well.
Thanks for pointing it out, I believe we can simply move what is considered feature request in the issue?
Thanks.

@jlquaccia
Copy link
Collaborator Author

Thanks @patmmccann, that makes sense and I will remember that for future PR's as well going forward!

@jlquaccia
Copy link
Collaborator Author

Updated the default logic around which bidders get their iframe/fetch url called. If no bidders are configured then no bidder resource gets called by default (the general doc.browsingTopics() call always gets made though). Also added my changes on top of @dgirardi 's latest change in the topicsFpdModule.js file.

Could I get another code review when everyone gets a chance?

@jlquaccia
Copy link
Collaborator Author

Reverted back to previous topics bidder default logic (as we have decided here.. #9982)

@patmmccann patmmccann merged commit 3fba6bd into prebid:master Aug 9, 2023
2 checks passed
santii7395 pushed a commit to themaven-net/Prebid.js that referenced this pull request Aug 28, 2023
* progress

* added support for topics api header fetch requests

* reverted some changes and comments

* refactored how topics are set in ls so that all included instead of just first indecx

* reverted a few minor changes

* formatting change

* reverted back to previous default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants