New Analytics Adapter: PubxAi #3863
Conversation
PubxAI Analytics adapter
|
Hi @tej656, when you have time, please merge with master and resolve conflicts. Also, we recently released PBS 3.0, more specifically v3.1.0, which updates Prebid Server package import references throughout the project from As a result, after merging with master, please ensure all Prebid Server package import references in the files you’ve changed are |
|
@bsardo i have updated the merge conflicts. |
analytics/build/build.go
Outdated
| @@ -43,6 +44,26 @@ func New(analytics *config.Analytics) analytics.Runner { | |||
| glog.Errorf("Could not initialize PubstackModule: %v", err) | |||
| } | |||
| } | |||
| glog.Info("pubxai.Enabled: ", analytics.Pubxai.Enabled) | |||
There was a problem hiding this comment.
Is this log line necessary? Do the other analytics adapters do this?
analytics/build/build.go
Outdated
| clock.New(), | ||
| ) | ||
| if err == nil { | ||
| glog.Infof("PubxaiModule initialized") |
There was a problem hiding this comment.
Nitpick: No need to use Infof for formatting for a static string.
analytics/pubxai/config/config.go
Outdated
| return nil, fmt.Errorf("fail to parse the module args, arg=analytics.pubxai.configuration_refresh_delay: %v", err) | ||
| } | ||
|
|
||
| endpointUrl, err := url.Parse(endpoint + "/config?pubxId=" + pubxId) |
There was a problem hiding this comment.
You must sanitize the user provided pubxId string when including it as a query parameter.
| @@ -0,0 +1,63 @@ | |||
| // Code generated by MockGen. DO NOT EDIT. | |||
There was a problem hiding this comment.
Can you use the testify mock library? That's our standard in this project.
There was a problem hiding this comment.
sure, updated all the mocks to use testify mock
| close(stop) | ||
| // Ensure task stops correctly | ||
| time.Sleep(2 * time.Second) | ||
| assert.NotNil(t, configChan) |
There was a problem hiding this comment.
Do not use sleep statements in tests. Instead, use channel messages or other definitive sync mechanisms. I'm not willing to add 4 seconds to this projects test runtime here.
There was a problem hiding this comment.
updated the tests to remove sleeps
| auctionBids, winningBids := processorService.ProcessLogData(ao) | ||
| assert.Nil(t, auctionBids) | ||
| assert.Nil(t, winningBids) | ||
| } |
There was a problem hiding this comment.
Kindly reformat to Go table driven tests.
There was a problem hiding this comment.
updated all the tests to Go table driven tests.
analytics/pubxai/utils/utils.go
Outdated
| Timestamp int64 `json:"timestamp"` | ||
| } | ||
|
|
||
| type UtilsService interface { |
There was a problem hiding this comment.
Why do utils need to be a service?
There was a problem hiding this comment.
this was for mainly for the Unit tests, but anyway, since not using gomock anymore, utils is not a service anymore
util/useragentutil/useragentutil.go
Outdated
| const ( | ||
| DEVICE_DESKTOP = iota | ||
| DEVICE_MOBILE | ||
| DEVICE_TABLET |
There was a problem hiding this comment.
Nitpick: Following Go naming standards, use MixedCaps here instead of SCREAMING_CAPS.
analytics/pubxai/utils/utils.go
Outdated
| type FloorData struct { | ||
| FetchStatus string `json:"fetchStatus"` | ||
| FloorProvider string `json:"floorProvider"` | ||
| Location string `json:"location"` | ||
| ModelVersion string `json:"modelVersion"` | ||
| NoFloorSignaled bool `json:"noFloorSignaled"` | ||
| SkipRate int64 `json:"skipRate"` | ||
| Skipped bool `json:"skipped"` | ||
| SkippedReason string `json:"skippedReason"` | ||
| } |
There was a problem hiding this comment.
Any specific reason to have duplicate struct ?
FloorDetail struct is having same fields
There was a problem hiding this comment.
aah, my bad, this was not used, removed it
Update mocking and Testing Framework changes
remove log statement
No description provided.