Stored response fetcher (with new func for stored resp) #2099

VeronikaSolovei9 · 2021-12-01T06:55:23Z

Stored response fetcher implementation:
-Added new FetchResponses function to Fetcher interface and all implementations
-Added new config object to store fetch responses configuration, assuming all stored responses will be stored in a new table.
-Functionality in this PR will be used in further stored responses feature implementation, so for now stored response fetcher does nothing - just exists and never gets called. Future places to call storedRespFetcher.FetchResponses are auction and amp endpoints.
-Initialization of Stored response fetcher happens in router.go -> func New -> storedRequestsConf.NewStoredRequests
-There are 2 actual implementations of FetchResponses function (others are just stubs to satisfy interface contract):
-- in db_fetcher/fetcher.go -> func (fetcher *dbFetcher) FetchResponses - place where stored responses downloads from DataBase
-- in stored_requests/fetcher.go -> func (f *fetcherWithCache) FetchResponses - place where stored responses get and set to in memory cache
-Polling functionality update - download newly created stored responses to in-memory cache: database.go -> fetchDelta() -> sendEvents (this is another place where data downloads from DB)

This PR is a first step to implement Prebid server stored responses feature

TODO: add metrics for stored responses - will be done in separate PR

Note: this code was tested using local Database. In order to enable it in prod new table should be created.

AlexBVolcy · 2021-12-08T17:16:51Z

Hey @VeronikaSolovei9, I was wondering if you could update the description of this PR to be a little more detailed about what this PR accomplishes? I think it would be helpful for me while reviewing :)

AlexBVolcy · 2021-12-13T01:24:12Z

stored_requests/backends/db_fetcher/fetcher.go

+
+	rows, err := fetcher.db.QueryContext(ctx, query, idInterfaces...)
+	if err != nil {
+		fmt.Println(err.Error(), query)


Do we need to be printing the error to console, instead of just returning this error?

Yes, updated :)

AlexBVolcy · 2021-12-13T01:26:15Z

stored_requests/backends/db_fetcher/fetcher.go

+		var data []byte
+		var dataType string
+
+		// Fixes #338


Nitpick, i don't think we need this comment here for the merge?

AlexBVolcy · 2021-12-13T01:26:33Z

stored_requests/backends/db_fetcher/fetcher.go

+		storedData[id] = data
+	}
+
+	// Fixes #338


Same point as above, i think this comment may not be necessary.

AlexBVolcy · 2021-12-13T01:26:59Z

stored_requests/backends/db_fetcher/fetcher.go

+
+	// Fixes #338
+	if rows.Err() != nil {
+		fmt.Println(rows.Err())


I don't think you need to print the error to terminal.

Right, removed

AlexBVolcy · 2021-12-15T17:32:41Z

config/stored_requests_test.go

+	expected := buildQueryResponses(sampleResponsesQueryTemplate, 0)
+	assertStringsEqual(t, query, expected)
+}
+


This might be a nitpick, but I see these tests from line 46-64, are all testing the same function but with different cases (i.e. No IDs vs Many IDs). This can be refactored to be one table driven test with many test cases within it, which is something I prefer, but of course if you'd like to keep it as it is then no worries.

Right, I followed the existing pattern, but yes, agree, this should be a table drive test. Updated.

AlexBVolcy · 2021-12-15T17:39:24Z

stored_requests/backends/db_fetcher/fetcher.go

 	"github.com/lib/pq"

 	"github.com/golang/glog"
 	"github.com/prebid/prebid-server/stored_requests"
 )

-func NewFetcher(db *sql.DB, queryMaker func(int, int) string) stored_requests.AllFetcher {
+func NewFetcher(db *sql.DB, queryMaker func(int, int) string, responseQueryMaker func(int) string) stored_requests.AllFetcher {


Just for my knowledge, what is happening here in this function signature where I read func(int) in the function signature. Is another function being called in relation to responseQueryMaker in the NewFetcher function signature?

Correct. There are two different functions to build queries.
Please refer to the config/stored_requests.go file.

func (cfg *PostgresFetcherQueries) MakeQuery(numReqs int, numImps int) (query string) { return resolve(cfg.QueryTemplate, numReqs, numImps) } func (cfg *PostgresFetcherQueries) MakeQueryResponses(numIds int) (query string) { return resolveQueryResponses(cfg.QueryTemplate, numIds) } func resolve(template string, numReqs int, numImps int) (query string) { numReqs = ensureNonNegative("Request", numReqs) numImps = ensureNonNegative("Imp", numImps) query = strings.Replace(template, "%REQUEST_ID_LIST%", makeIdList(0, numReqs), -1) query = strings.Replace(query, "%IMP_ID_LIST%", makeIdList(numReqs, numImps), -1) return } func resolveQueryResponses(template string, numIds int) (query string) { numIds = ensureNonNegative("Response", numIds) query = strings.Replace(template, "%ID_LIST%", makeIdList(0, numIds), -1) return }

There are two different resolvers for db queries. We need them to support stored responses and stored requests db fetch functionality.
For stored requests there is a query that fetches both "Request" and "Imp" in one run using union and it has two placeholders: "%REQUEST_ID_LIST%" and "%IMP_ID_LIST%".
It's complicated and messy to reuse it for responses.
For responses there is another query that fetches just one type of data in one run, it's simpler and eventually we plan to replace complicated query with union to a simple query like we have for responses. Also there is no bundle to data type like REQUEST_ID_LIST or IMP_ID_LIST. For now I can see this is the cleanest way to support it and make code and configs more or less readable.

AlexBVolcy · 2021-12-15T17:41:10Z

stored_requests/backends/db_fetcher/fetcher_test.go

+	assertHasData(t, storedResponses, "resp-id-1", `{"resp":false,"value":1}`)
+	assertHasData(t, storedResponses, "resp-id-2", `{"resp":true,"value":2}`)
+}
+


Similar comment to above, where I think the TestGoodRepsonseFetchResponses, TestPartialResponse, and TestEmptyResponse tests could be consolidated into one table driven test.

AlexBVolcy · 2021-12-15T17:41:19Z

stored_requests/backends/db_fetcher/fetcher_test.go

+	assertMapLength(t, 1, storedResponses)
+	assertHasData(t, storedResponses, "stored-resp-id", "{}")
+}
+


Similar comment to above, where I think the TestGoodRepsonseFetchResponses, TestPartialResponse, and TestEmptyResponse tests could be consolidated into one table driven test.

AlexBVolcy · 2021-12-15T17:41:26Z

stored_requests/backends/db_fetcher/fetcher_test.go

+	assertErrorCount(t, 0, errs)
+	assertMapLength(t, 0, storedResponses)
+}
+


Similar comment to above, where I think the TestGoodRepsonseFetchResponses, TestPartialResponse, and TestEmptyResponse tests could be consolidated into one table driven test.

AlexBVolcy · 2021-12-15T17:43:36Z

stored_requests/events/http/http.go

+//   },
+//   "responses": {
+//     "resp1": { ... stored data for resp1 ... },
+//     "resp2": { ... stored data for resp2 ... },


Is this commented out code still needed?

Yes, this is a valid comment.

AlexBVolcy · 2021-12-15T17:44:28Z

stored_requests/fetcher.go

+	// Record cache hits for stored responses
+	//f.metricsEngine.RecordStoredReqCacheResult(metrics.CacheHit, len(ids)-len(leftoverResp))
+	// Record cache misses for stored responses
+	//f.metricsEngine.RecordStoredImpCacheResult(metrics.CacheMiss, len(leftoverResp))


Is this commented out code still needed?

bsardo

This is looking good. I've gone through all of the logic changes and have just a few comments. I still need to finish going through a few more tests to complete the review which I will do shortly.

bsardo · 2021-12-18T04:17:22Z

stored_requests/backends/db_fetcher/fetcher.go

+	if responseQueryMaker == nil {
+		glog.Fatalf("The Postgres Stored Response Fetcher requires a responseQueryMaker function. Please report this as a bug.")
+	}


It seems odd to me that the responseQueryMaker is required to be not nil here because this function is not only used to create a database-backed fetcher for the stored response fetcher, but also the stored requests fetcher.
From what I can tell, if you're in a test environment where you want stored responses, the host would configure the stored responses fetcher but not the stored requests fetcher and they would define both the requests and response queries for the response fetcher. If you're in a prod environment, the host would configured the stored requests fetcher but not the stored response fetcher, and in that case, the host should only be required to configure the requests query but not a response query. Does that make sense? Was that how you envisioned the configuration working? If so, then I think a change is needed here because I think this will result in a fatal error in the production scenario and I don't think we want to require the host to define a response query.

This code is not clear enough and needs to be separate in future.
Now it works with any kind of configuration because function is hardcoded and logic in upstream function determines what kind of data we want to fetch:
/stored_requests/config/config.go -> func newFetcher :

//stored imps, req, videos, responses. If config exists each of them will have a separate fetcher. if cfg.Postgres.FetcherQueries.QueryTemplate != "" { idList = append(idList, db_fetcher.NewFetcher(db, cfg.Postgres.FetcherQueries.MakeQuery, cfg.Postgres.FetcherQueries.MakeQueryResponses)) }

If stored response config is not defined - this part of code will be skipped.
Hope this answers your question.
But Idk why do we need to check if queryMaker and responseQueryMaker are defined, they are hardcoded upstream. I followed existing code pattern.

Ah gotcha. I see that they are always not nil.

bsardo · 2021-12-20T14:11:22Z

config/stored_requests.go

@@ -401,6 +412,8 @@ type InMemoryCache struct {
 	RequestCacheSize int `mapstructure:"request_cache_size_bytes"`
 	// ImpCacheSize is the max number of bytes allowed in the cache for Stored Imps. Values <= 0 will have no limit
 	ImpCacheSize int `mapstructure:"imp_cache_size_bytes"`
+	// ResponsesCacheSize is the max number of bytes allowed in the cache for Stored Responses. Values <= 0 will have no limit
+	RespCacheSize int `mapstructure:"resp_cache_size_bytes"`


Can you add defaults for this field in config/config.go for each fetcher similar to what we do for request_cache_size_bytes and imp_cache_size_bytes?

Good catch, added

bsardo · 2021-12-20T15:16:09Z

stored_requests/events/api/api_test.go

-		Accounts: memory.NewCache(256*1024, -1, "Account"),
+		Requests:  memory.NewCache(256*1024, -1, "Request"),
+		Imps:      memory.NewCache(256*1024, -1, "Imp"),
+		Responses: memory.NewCache(256*1024, -1, "Responseds"),


Super nitpick: typo should be "Responses"

Ooof, modified!

bsardo

The tests look good, just a couple of comments.

bsardo · 2021-12-20T16:27:02Z

config/stored_requests_test.go

+	}
+
+	for _, test := range testCases {
+		query := buildQueryResponses(sampleResponsesQueryTemplate, test.inputRespNumber)


Super nitpick and maybe more of a matter of personal preference: I don't see the value in having a level of indirection in this test by introducing a function buildQueryResponses unless you're planning to reuse that function in some other test. Why not just keep everything inline here and instead write:

// build query response cfg := PostgresFetcherQueries{QueryTemplate: sampleResponsesQueryTemplate} query := cfg.MakeQueryResponses(test.inputRespNumber)

I'm ok if you want to keep it as is. I just think that tests are easier to understand if they have as little indirection as possible.

Modified. It was a copy-paste from stored requests tests

bsardo · 2021-12-20T16:56:09Z

config/stored_requests_test.go

 	assertErrsExist(t, (&InMemoryCache{
 		Type:             "lru",
 		RequestCacheSize: 1000,
 		ImpCacheSize:     0,
+		RespCacheSize:    0,
 	}).validate(RequestDataType, nil))


Does it make more sense to isolate the error to the ImpCacheSize and RespCacheSize here so instead of this assertErrsExist where both ImpCacheSize and RespCacheSize are 0, we introduce two assertErrsExists where each has only one of them set to 0 like so:

assertErrsExist(t, (&InMemoryCache{ Type: "lru", RequestCacheSize: 1000, ImpCacheSize: 0, RespCacheSize: 1000, }).validate(RequestDataType, nil)) assertErrsExist(t, (&InMemoryCache{ Type: "lru", RequestCacheSize: 1000, ImpCacheSize: 1000, RespCacheSize: 0, }).validate(RequestDataType, nil))

Not sure I understand this. Can we discuss it offline?

Fixed. Sorry I was overthinking about this :)

AlexBVolcy

I just have a couple more small comments. This is looking good!

AlexBVolcy · 2022-01-06T02:44:32Z

stored_requests/config/config.go

+	accountsFetcher stored_requests.AccountFetcher,
+	categoriesFetcher stored_requests.CategoryFetcher,
+	videoFetcher stored_requests.Fetcher,
+	storedRespFetcher stored_requests.Fetcher) {
 	// TODO: Switch this to be set in config defaults


Are these comments still needed here?

AlexBVolcy · 2022-01-06T02:44:57Z

stored_requests/backends/db_fetcher/fetcher.go

+		}
+	}()
+
+	storedData := make(map[string]json.RawMessage, len(ids))


You use len(ids) a few times in this function, lines 101, 105, 106, and here 121. Is the length of the ids array changing at all here? If no, you could just extract len(ids) to a variable, and use that variable instead, just to improve some efficiency here.

len(arr) is just a memory reading operation. Array already has this value and it doesn't need to be re-evaluated.

* 'master' of https://github.com/wwwyyy/prebid-server: GDPR: pass geo based on special feature 1 opt-in (prebid#2122) Adyoulike: Fix adapter errors if no content (prebid#2134) Update adyoulike.yaml (prebid#2131) Add ORTB 2.4 schain support (prebid#2108) Stored response fetcher (with new func for stored resp) (prebid#2099) Adot: Add OpenRTB macros resolution (prebid#2118) Rubicon adapter: accept accountId, siteId, zoneId as strings (prebid#2110) Operaads: support multiformat request (prebid#2121) Update go-gdpr to v1.11.0 (prebid#2125) Adgeneration: Update request to include device and app related info in headers and query params (prebid#2109) Adnuntius: Read device information from ortb Request (prebid#2112) New Adapter: apacdex and Remove Adapter: valueimpression (prebid#2097) New Adapter: Compass (prebid#2102) Orbidder: bidfloor currency handling (prebid#2093)

* Stored resp fetcher * Stored resp fetcher unit tests * Rebase on upstream * Code clean up * Unit tests clean up * Unit tests clean up * Code clean up * Minor unit test fix * Deleted old comment

VeronikaSolovei9 marked this pull request as draft December 1, 2021 06:55

This was referenced Dec 2, 2021

Stored response fetcher (with new param) #2062

Closed

Renamed FetchRequests to Fetch #2086

Closed

mansinahar requested review from AlexBVolcy and bsardo December 6, 2021 18:13

mansinahar assigned bsardo and AlexBVolcy Dec 6, 2021

VeronikaSolovei9 force-pushed the stored_resp_fetcher_v2 branch from 4229b8b to 8dfa4a1 Compare December 6, 2021 19:25

VeronikaSolovei9 added 3 commits December 8, 2021 01:41

Stored resp fetcher

a618772

Stored resp fetcher unit tests

d7eb26a

Rebase on upstream

a27dbc2

VeronikaSolovei9 force-pushed the stored_resp_fetcher_v2 branch from b9609bf to a27dbc2 Compare December 8, 2021 09:49

VeronikaSolovei9 marked this pull request as ready for review December 9, 2021 02:19

AlexBVolcy reviewed Dec 13, 2021

View reviewed changes

Code clean up

20d1523

AlexBVolcy reviewed Dec 15, 2021

View reviewed changes

VeronikaSolovei9 added 2 commits December 15, 2021 12:21

Unit tests clean up

2ee96dc

Unit tests clean up

5416bc6

bsardo reviewed Dec 20, 2021

View reviewed changes

Code clean up

3849bd0

VeronikaSolovei9 force-pushed the stored_resp_fetcher_v2 branch from aa02b19 to 3849bd0 Compare December 21, 2021 08:45

Minor unit test fix

5477324

AlexBVolcy reviewed Jan 6, 2022

View reviewed changes

Deleted old comment

7d8bb6f

bsardo approved these changes Jan 10, 2022

View reviewed changes

AlexBVolcy approved these changes Jan 10, 2022

View reviewed changes

AlexBVolcy merged commit ec1df96 into master Jan 10, 2022

SyntaxNode deleted the stored_resp_fetcher_v2 branch March 13, 2023 13:55

Stored response fetcher (with new func for stored resp) #2099

Stored response fetcher (with new func for stored resp) #2099

Conversation

VeronikaSolovei9 commented Dec 1, 2021 • edited Loading

AlexBVolcy commented Dec 8, 2021 • 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

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

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

bsardo 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

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

bsardo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy left a comment • 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

VeronikaSolovei9 commented Dec 1, 2021 •

edited

Loading

AlexBVolcy commented Dec 8, 2021 •

edited

Loading

AlexBVolcy left a comment •

edited

Loading