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

Stored response fetcher (with new param) #2062

Closed
wants to merge 10 commits into from

Conversation

VeronikaSolovei9
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 commented Oct 27, 2021

Stored response fetcher
Fetch

@VeronikaSolovei9
Copy link
Contributor Author

VeronikaSolovei9 commented Oct 29, 2021

Initial working implementation is ready for review.
Questions:

  1. There are two types of data we fetch from DB now: "imp" and "request". Should I add new type "response" and all related functionality (update database.go -> sendEvents and fetcher.go -> FetchRequests functions)
  2. I reused existing stored_requests.Fetcher for stored responses. Should I rename it or create new one?
  3. There are no unit tests for this functionality. Do I miss something?

@VeronikaSolovei9 VeronikaSolovei9 marked this pull request as ready for review October 29, 2021 17:50
@mansinahar mansinahar requested review from bsardo and hhhjort November 1, 2021 17:25
@hhhjort
Copy link
Collaborator

hhhjort commented Nov 1, 2021

  1. There are more than two types, there are also "video" and "category" items that can be fetched. The one thing they have in common is that they all fetch json blobs. Imp and request were initially intertwined, as when initially implemented there was no other type of fetcher contemplated. Since then, we have been trying to detangle them as we add more fetchers, and have a common codebase that all fetchers can use. So reuse existing code when it makes sense, and add response specific code as needed.
  2. In router.go, I see a separate storedRespFetcher object, which is the point where we need a separate object. As I said above, if we can reuse fetcher code, we should do so. We might consider renaming the module from stored_requests to stored_objects, since the fetcher framework has already expanded beyond requests. But that is probably out of scope for this PR.
  3. The purpose of this code is to provide the ability to do end to end tests without external dependencies. So you should be able to set up a test with an auction object configured with a simple file based stored response fetcher, feed it a request that invokes the stored response, and verify that it returns the expected bids.

@@ -86,7 +86,8 @@ func NewAmpEndpoint(
bidderMap,
nil,
nil,
ipValidator}).AmpAuction), nil
ipValidator,
empty_fetcher.EmptyFetcher{}}).AmpAuction), nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we using an empty fetcher? We should be able to use stored responses on the AMP endpoint I would imagine.

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'm not sure about this yet. Do we need to support stored response functionality in amp endpoint?

@@ -109,6 +111,7 @@ type endpointDeps struct {
cache prebid_cache_client.Client
debugLogRegexp *regexp.Regexp
privateNetworkIPValidator iputil.IPValidator
storedRespFetcher stored_requests.Fetcher
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is the code that checks for the stored response signal, and then bypasses sending a request to the bidder and just providing the stored response to the flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is only about stored response fetcher.
We decided to have smaller PRs that are easy to review rather than having big and complicated PRs.
I understand this fetcher is not connected to anything and has no usages.
I'll add actual code related to stored responses in another PR.

@mansinahar mansinahar marked this pull request as draft November 4, 2021 17:17
@VeronikaSolovei9 VeronikaSolovei9 changed the title Stored response fetcher Stored response fetcher (with new param) Nov 29, 2021
@mansinahar
Copy link
Contributor

Closing this in favor of #2099. Will reopen if needed.

@mansinahar mansinahar closed this Dec 2, 2021
@SyntaxNode SyntaxNode deleted the stored_response_fetcher branch March 13, 2023 13:55
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.

4 participants