New Module: 51Degrees #3650
New Module: 51Degrees #3650
Conversation
Co-authored-by: Sarana-Anna <[email protected]> Co-authored-by: James Rosewell <[email protected]>
* expose watch_file_system option * expose make_temp_copy option * improved performance
# Conflicts: # go.mod # go.sum
after mergin master
|
Hello. Thanks for your patience. I just want to give you a quick status update. For various reasons, we are going to need a couple more weeks before we're able to tackle this. There's a lot here so it requires a significant time investment to review, and on top of that, we may also need to perform a security review of your library and quite possibly lock the version in PBS. Stay tuned. |
|
@bsardo - Thank you. To help make the review process more efficient for everyone I'd be happy to setup a virtual meeting with the reviewers to answer questions and clarify feedback in real time. Please let me know if this is something you or other reviewers would like to arrange. |
remove fields that could be overwritten
| } | ||
| } | ||
|
|
||
| func (x *AccountValidator) IsWhiteListed(cfg Config, req []byte) bool { |
There was a problem hiding this comment.
Nitpick: Consider renaming to "IsAllowed" to align with the "AccountFilter.AllowList" terminology and to use use more inclusive language.
| func (x EvidenceFromRequestHeadersExtractor) extractEvidenceStrings(r *http.Request, keys []dd.EvidenceKey) []StringEvidence { | ||
| evidence := make([]StringEvidence, 0) | ||
| for _, e := range keys { | ||
| lowerKey := strings.ToLower(e.Key) |
There was a problem hiding this comment.
Nitpick / Performance: There is no need to convert the key to lower case r.Header.Get is case insensitive.
| package device_detection | ||
|
|
||
| // Contains checks if a string is in a slice of strings | ||
| func Contains(source []string, item string) bool { |
There was a problem hiding this comment.
Consider using the built-in slices.Contains added in Go 1.21.
| github.com/tidwall/gjson v1.17.1 // indirect | ||
| github.com/tidwall/match v1.1.1 // indirect | ||
| github.com/tidwall/pretty v1.2.0 // indirect | ||
| github.com/tidwall/sjson v1.2.5 // indirect |
There was a problem hiding this comment.
As discussed, I ran some gjson vs jsonparser benchmark tests and while jsonparser is slightly faster for more complex cases with less bytes per operation, we consider the difference small enough that we are good with using gjson in this module. We do also like that gjson has a sister library sjson that you can use. The jsonparser library has a set feature but it is experimental and we have found that it is buggy.
Here are the benchmark test results:
goos: darwin
goarch: amd64
pkg: github.com/prebid/prebid-server/v2/modules/fiftyone_degrees/device_detection
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkSingleTopLevelGJSON-12 3016309 398.3 ns/op 80 B/op 1 allocs/op
BenchmarkSingleTopLevelJSONParser-12 2739925 426.3 ns/op 0 B/op 0 allocs/op
BenchmarkSingleNestedGJSON-12 2841622 423.3 ns/op 24 B/op 1 allocs/op
BenchmarkSingleNestedJSONParser-12 2601579 406.7 ns/op 0 B/op 0 allocs/op
BenchmarkManyNestedGJSON-12 622218 1928 ns/op 624 B/op 6 allocs/op
BenchmarkManyNestedJSONParser-12 769302 1388 ns/op 304 B/op 8 allocs/op
See the attached file containing the benchmark tests.
json_benchmark_test.txt
|
|
||
| ### Account-Level Config | ||
|
|
||
| To start using current module in PBS-Java you have to enable module and add `fiftyone-devicedetection-entrypoint-hook` and `fiftyone-devicedetection-raw-auction-request-hook` into hooks execution plan inside your config file: |
| fiftyone_degrees: | ||
| device_detection: | ||
| enabled: true | ||
| data-file: |
There was a problem hiding this comment.
Typo: data-file --> data_file
| var cfg Config | ||
| ncfg, err := ParseConfig(rawConfig) |
There was a problem hiding this comment.
Nitpick: I think you can get rid of cfg and just use ncfg and get rid of the cfg = ncfg assignment on line 46.
| // handleAuctionEntryPointRequestHook is a hookstage.HookFunc that is used to handle the auction entrypoint request hook. | ||
| func handleAuctionEntryPointRequestHook(cfg Config, payload hookstage.EntrypointPayload, deviceDetector deviceDetector, evidenceExtractor evidenceExtractor, accountValidator accountValidator) (result hookstage.HookResult[hookstage.EntrypointPayload], err error) { | ||
| // if account/domain is not whitelisted, return failure | ||
| if accountValidator.IsWhiteListed(cfg, payload.Body) != true { |
There was a problem hiding this comment.
Nitpick: go best practice would be to use the not operator instead of != true:
if !accountValidator.IsWhiteListed(cfg, payload.Body) {
| // fiftyOneDtToRTB converts a 51Degrees device type to an OpenRTB device type. | ||
| // If the device type is not recognized, it defaults to PC. |
There was a problem hiding this comment.
Can you move this comment down so it is right above the fiftyOneDtToRTB function?
| func (x DeviceInfoExtractor) getValue( | ||
| results Results, | ||
| propertyName DeviceInfoProperty) string { |
There was a problem hiding this comment.
Nitpick: please put the whole function signature on the same line.
| ",", | ||
| ) | ||
| if err != nil { | ||
| log.Printf("ERROR: Failed to get results values string.") |
There was a problem hiding this comment.
I'm curious why you're using log here and in a few other places instead of glog. I think we should use glog throughout, in which case this would probably be a glog.Errorf.
| ",", | ||
| ) | ||
| if err != nil { | ||
| log.Printf("ERROR: Failed to get results values string.") |
There was a problem hiding this comment.
Also if you do get this error, it looks like the error would get returned via lines 120-122. Would it be better to just early return here so it is more obvious?
|
|
||
| // Extract extracts the account information from the payload | ||
| // The account information is extracted from the publisher id or site publisher id | ||
| func (x AccountInfoExtractor) Extract(payload []byte) *AccountInfo { |
There was a problem hiding this comment.
As a general comment applicable to all public functions and variables, please assess whether these need to be exported. I don't think most of these are meant to be accessed outside of the device_detection package, in which case they should begin with a lower case letter.
|
Thanks for this review @SyntaxNode and @bsardo! The recent push addresses Scott's comments, we'll work on addressing Brian's comments tomorrow. |
There was a problem hiding this comment.
I've finished going through the updates made in response to my last review as well as all of the tests. This is very close. Most of my comments are nitpicks and quick tweaks. The more time consuming ones are recommendations to move some of the tests to a table-based format with additional test cases. I've provided some of those to expedite things. Hopefully we can get this into this week's release.
@SyntaxNode do you have time this week to review?
| ",", | ||
| ) | ||
| if err != nil { | ||
| glog.Errorf("ERROR: Failed to get results values string.") |
There was a problem hiding this comment.
Nitpick: I think you can remove ERROR: from the message since glog will handle indicating that fact.
| onpremise.WithProperties([]string{ | ||
| "HardwareVendor", | ||
| "HardwareName", | ||
| "deviceType", |
There was a problem hiding this comment.
Is this supposed to be "DeviceType",?
| func (x defaultDeviceDetector) getDeviceInfo(evidence []onpremise.Evidence, ua string) (*deviceInfo, error) { | ||
| results, err := x.engine.Process(evidence) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "Failed to Process evidence") |
There was a problem hiding this comment.
Should Process be lowercase?
| const ( | ||
| deviceInfoHardwareVendor deviceInfoProperty = "HardwareVendor" | ||
| deviceInfoHardwareName deviceInfoProperty = "HardwareName" | ||
| deviceInfoDeviceType deviceInfoProperty = "deviceType" |
There was a problem hiding this comment.
"deviceType" should be "DeviceType"?
There was a problem hiding this comment.
Yes, thanks again! It seems like some renaming went wrong
| er := fmt.Sprintf("error hydrating fields %s", err) | ||
| glog.Error(er) | ||
| return rawPayload, hookexecution.NewFailure(er) |
There was a problem hiding this comment.
I'm curious if we really want to log an error here. There are plenty of other hook execution failures caught both in this file and in the module where we don't do that. Thoughts?
There was a problem hiding this comment.
that's a leftover from the previous version of code, before your first review.
it had only the log part: glog.Errorf("error hydrating fields %s", err) but you suggested returning a hookexecution Failure error here instead of just logging. I'll remove the log here as it is redundant now.
| ) | ||
| } | ||
|
|
||
| func TestModule_HandleRawAuctionHookDoesNotHaveModuleCtx(t *testing.T) { |
There was a problem hiding this comment.
Nitpick: TestHandleRawAuctionHookNoCtx
| assert.Errorf(t, err, "entrypoint hook was not configured") | ||
| } | ||
|
|
||
| func TestModule_TestModule_HandleRawAuctionHookExtractError(t *testing.T) { |
There was a problem hiding this comment.
Nitpick: TestHandleRawAuctionHookExtractError
| module := Module{} | ||
|
|
||
| _, err := module.HandleRawAuctionHook( | ||
| nil, hookstage.ModuleInvocationContext{}, |
There was a problem hiding this comment.
Nitpick: Please put these two parameters on separate lines.
|
|
||
| } | ||
|
|
||
| func TestModule_HandleRawAuctionHookEnrichment(t *testing.T) { |
There was a problem hiding this comment.
Nitpick: TestHandleRawAuctionHookEnrichment
| assert.Errorf(t, err, "error getting device info") | ||
| } | ||
|
|
||
| func TestModule_HandleRawAuctionHookEnrichmentWithErrors(t *testing.T) { |
There was a problem hiding this comment.
Nitpick: TestHandleRawAuctionHookEnrichmentWithErrors
|
Hi @bsardo the latest push should address the above comments - please check. As always thank you for such a thorough review! Not sure we made it into the release, but hopefully we are close. |
| name: "allow list is nil", | ||
| allowList: nil, | ||
| expectedResult: true, | ||
| }, | ||
| { | ||
| name: "allow list contains multiple", |
There was a problem hiding this comment.
Nitpick: for all test cases here, replace spaces in name field with underscores which is what t.Run will do when it encounters spaces (e.g. "allow list contains multiple" --> "allow_list_contains_multiple"). This will make it easy to find a failing test via copy and search.
| name: "Success path", | ||
| engineResponse: &dd.ResultsHash{}, | ||
| engineError: nil, | ||
| expectedResult: &deviceInfo{ | ||
| DeviceId: "123", | ||
| }, | ||
| expectedError: "", | ||
| }, | ||
| { | ||
| name: "Error path", |
There was a problem hiding this comment.
Nitpick: for all test cases here, replace spaces in name field with underscores which is what t.Run will do when it encounters spaces (e.g. "Error path" --> "Error_path"). This will make it easy to find a failing test via copy and search.
| expectedValue string | ||
| }{ | ||
| { | ||
| name: "from SUA tag", |
There was a problem hiding this comment.
Nitpick: for all test cases here, replace spaces in name field with underscores which is what t.Run will do when it encounters spaces (e.g. "from SUA tag" --> "from_SUA_tag"). This will make it easy to find a failing test via copy and search.
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestExtractEvidenceStrings(t *testing.T) { |
There was a problem hiding this comment.
Nitpick: for all test cases here, replace spaces in name field with underscores which is what t.Run will do when it encounters spaces (e.g. "Ignored query evidence" --> "Ignored_query_evidence"). This will make it easy to find a failing test via copy and search.
Co-authored-by: James Rosewell <[email protected]> Co-authored-by: Marin Miletic <[email protected]> Co-authored-by: Sarana-Anna <[email protected]> Co-authored-by: Eugene Dorfman <[email protected]> Co-authored-by: Krasilchuk Yaroslav <[email protected]>
This reverts commit 2606e75.
Type of change
Description of change
51Degrees module enriches an incoming OpenRTB request with 51Degrees Device Data.
51Degrees module sets the following fields of the device object:
make,model,os,osv,h,w,ppi,pxratio- interested bidder adapters may use these fields as needed. In addition the module setsdevice.ext.fiftyonedegrees_deviceIdto a permanent device ID which can be rapidly looked up in on premise data exposing over 250 properties including the device age, chip set, codec support, and price, operating system and app/browser versions, age, and embedded features.The module is based on the open source 51Degrees device detection library: https://github.com/51Degrees/device-detection-go.
Integration details are specified in the
modules/fiftyone_degrees/device_detection/README.md.Maintainer contacts:
[email protected]