Floors: Dynamic fetch

[pm-nikhil-vaidya]

Design document of dynamic fetch mechanism
https://docs.google.com/document/d/1Lb302nt6nDYQxeflzISjtIWpE6ifExdoT8PSPm8YSKk/edit#heading=h.bxed3ptmj442

Issue + Specs: #1162

[AlexBVolcy]

I left this comment a few times, but saw it in many other places, but could you remove any blank lines from the beginning of functions?

[AlexBVolcy]

This looks great! Close to approving, some comments about test coverage. Could you also merge with master to fix the merge conflicts? Thanks!

[AlexBVolcy]

This looks good! I will wait for @bsardo's review before approving.

[pm-nikhil-vaidya]

@SyntaxNode
In the last conversation on the design implementation, we have decided that we can implement the normal go routine.
As we are going to modify the fetcher in later code optimisation i.e fetcher 2.0, can we go head with this implementation instead of changing this again. Even if we modify the implementation the code will be short lived so we can do the changes at the time of fetcher 2.0
Let me know your thoughts on this

[SyntaxNode]

@SyntaxNode In the last conversation on the design implementation, we have decided that we can implement the normal go routine. As we are going to modify the fetcher in later code optimisation i.e fetcher 2.0, can we go head with this implementation instead of changing this again. Even if we modify the implementation the code will be short lived so we can do the changes at the time of fetcher 2.0 Let me know your thoughts on this

Yes. That sounds perfect. We'll keep in close contact on the design of Fetchers 2.0 in the Fall to ensure it has good performance at scale, and investigate the worker pool proposal.

[hhhjort]

Please fix the merge conflict.

[pm-nikhil-vaidya]

Modified the code which addresses below points

1. Switched from go-cache to freecache
2. introduced httpclient and its config instead of using default http client
3. Used time interface from timeutil
4. introduced metrics engine instance in fetcher

[AlexBVolcy]

Looks good to me! Will wait for @guscarreon before approving.

[guscarreon]

Aside from the last comment above, we also need to address the merge conflicts.

[pm-nikhil-vaidya]

@guscarreon
Added retry mechanism if URL is long dead

Max retries are configurable through config file

type PriceFloorFetcher struct {
	HttpClient HTTPClient `mapstructure:"http_client"`
	CacheSize  int        `mapstructure:"cache_size_mb"`
	Worker     int        `mapstructure:"worker"`
	Capacity   int        `mapstructure:"capacity"`
	MaxRetries int        `mapstructure:"max_retries"`
}

This config is consumed when floor fetcher is initialised

floorFetcher := PriceFloorFetcher{
		pool:            pond.New(config.Fetcher.Worker, config.Fetcher.Capacity, pond.PanicHandler(workerPanicHandler)),
		fetchQueue:      make(FetchQueue, 0, 100),
		fetchInProgress: make(map[string]bool),
		configReceiver:  make(chan fetchInfo, config.Fetcher.Capacity),
		done:            make(chan struct{}),
		cache:           freecache.NewCache(config.Fetcher.CacheSize * 1024 * 1024),
		httpClient:      httpClient,
		time:            &timeutil.RealTime{},
		metricEngine:    metricEngine,
		maxRetries:      config.Fetcher.MaxRetries,
	}

The worker will try to fetch the floor data from url and if fetch fails then it will increment the retry count in fetchInfo else it will reset the count value

func (f *PriceFloorFetcher) worker(fetchConfig fetchInfo) {
	floorData, fetchedMaxAge := f.fetchAndValidate(fetchConfig.AccountFloorFetch)
	if floorData != nil {
		// Reset retry count when data is successfully fetched
		fetchConfig.retryCount = 0

		// Update cache with new floor rules
		cacheExpiry := fetchConfig.AccountFloorFetch.MaxAge
		if fetchedMaxAge != 0 {
			cacheExpiry = fetchedMaxAge
		}
		floorData, err := json.Marshal(floorData)
		if err != nil {
			glog.Errorf("Error while marshaling fetched floor data for url %s", fetchConfig.AccountFloorFetch.URL)
		} else {
			f.SetWithExpiry(fetchConfig.AccountFloorFetch.URL, floorData, cacheExpiry)
		}
	} else {
		fetchConfig.retryCount++
	}

	// Send to refetch channel
	if fetchConfig.retryCount < f.maxRetries {
		fetchConfig.fetchTime = f.time.Now().Add(time.Duration(fetchConfig.AccountFloorFetch.Period) * time.Second).Unix()
		fetchConfig.refetchRequest = true
		f.configReceiver <- fetchConfig
	}
}

[pm-nikhil-vaidya]

@guscarreon @SyntaxNode @bsardo @hhhjort @AlexBVolcy

I have addressed all the review comments. Kindly review and let me know if anything needs to be addressed so that we can finalise this implementation.

[guscarreon]

After data was fetched and cached successfully, do we need to refetch? What would be the purpose? To update the floorprice?
Or, should we stop refetching and, if the cacheExpiry is met, floor info us simply removed from cache and will be refetched whenever a new request comes in?

155   func (f *PriceFloorFetcher) worker(fetchConfig fetchInfo) {
156       floorData, fetchedMaxAge := f.fetchAndValidate(fetchConfig.AccountFloorFetch)
157       if floorData != nil {
158           // Reset retry count when data is successfully fetched
159           fetchConfig.retryCount = 0
160  
161           // Update cache with new floor rules
162           cacheExpiry := fetchConfig.AccountFloorFetch.MaxAge
163           if fetchedMaxAge != 0 {
164               cacheExpiry = fetchedMaxAge
165           }
166           floorData, err := json.Marshal(floorData)
167           if err != nil {
168               glog.Errorf("Error while marshaling fetched floor data for url %s", fetchConfig.AccountFloorFetch.URL)
169           } else {
170               f.SetWithExpiry(fetchConfig.AccountFloorFetch.URL, floorData, cacheExpiry)
171           }
    +         
    +         return
    +         
172       } else {
173           fetchConfig.retryCount++
174       }
175  
176       // Send to refetch channel
177       if fetchConfig.retryCount < f.maxRetries {
178           fetchConfig.fetchTime = f.time.Now().Add(time.Duration(fetchConfig.AccountFloorFetch.Period) * time.Second).Unix()
179           fetchConfig.refetchRequest = true
180           f.configReceiver <- fetchConfig
181       }
182   }
floors/fetcher.go

Thoughts?

[guscarreon]

Also what should be default value for retries?

I'm sorry I missed your question. Sure, 10 is fine

[pm-nikhil-vaidya]

The refetch mechanism is mainly introduced as this hosted floor data will be updated by ML algorithms , ML will constantly learn by applying particular floor value to request and based on bids it will increase or decrease the floor value, so yes we will require refetch mechanism.

[pm-nikhil-vaidya]

PRD also mentions about refetch mechanism
https://docs.google.com/document/d/1YAt00RfQaU2Ugciqw1LnirLgjyUz9ibfyjfxWUHR_P0/edit#heading=h.yglzagui7gzp

[guscarreon]

Thanks for addressing my feedback @pm-nikhil-vaidya . LGTM

[SyntaxNode]

Prebid Server 2.0 has been released and Go Module name has changed from github.com/prebid/prebid-server to github.com/prebid/prebid-server/v2, per Go versioning conventions.

Please merge the master branch and update all prebid-server import statements to reference the v2 name change. Thank you.

[pm-nikhil-vaidya]

@SyntaxNode @guscarreon

Updated to V2

[guscarreon]

LGTM

[AlexBVolcy]

LGTM