Added TransmitPreciseGeo activity handling in modules #3348

VeronikaSolovei9 · 2023-12-12T20:35:49Z

No description provided.

AlexBVolcy

This looks good! Just left one very minor nitpick.

AlexBVolcy · 2023-12-14T00:43:40Z

hooks/hookexecution/execution_test.go

@@ -49,13 +55,45 @@ func TestHandleModuleActivitiesBidderRequestPayload(t *testing.T) {
 				}},
 			},
 		},
+


Nitpick: can you remove this blank line?

I refactored func handleModuleActivities the way you proposed to refactor similar function in analytics activities to reduce nesting.

bsardo · 2023-12-14T17:50:04Z

hooks/hookexecution/executor.go

@@ -68,6 +68,7 @@ func NewHookExecutor(builder hooks.ExecutionPlanBuilder, endpoint string, me met
 		stageOutcomes:  []StageOutcome{},
 		moduleContexts: &moduleContexts{ctxs: make(map[string]hookstage.ModuleContext)},
 		metricEngine:   me,
+		account:        &config.Account{Privacy: config.AccountPrivacy{}},


What's the motivation for creating an empty account here instead of just keeping it as nil until an account is set on the hookExecutor using SetAccount? It seems like an attempt to avoid nil checks when executing each stage but I'm not sure that's a good idea.

Addressed. I changed handleModuleActivities function signature to accept account *config.Account instead of AccountPrivacy.

VeronikaSolovei9 · 2023-12-15T19:22:23Z

hooks/hookexecution/executor_test.go

@@ -760,7 +760,7 @@ func TestExecuteProcessedAuctionStage(t *testing.T) {
 		{
 			description:            "Stage execution can be rejected - and later hooks rejected",
 			givenPlanBuilder:       TestRejectPlanBuilder{},
-			givenAccount:           nil,
+			givenAccount:           account,


We need to pass account here, because we want to test that BidRequest will not change even if we restrict activities: getModuleActivities("foo", false, false) and payload for the hook will be modified.

If we add the account nil check in handleModuleActivities we shouldn't need to define an account here right? The way this test case is currently constructed, it seems like the only reason an account is being passed here now is to ensure PBS doesn't crash.

bsardo · 2024-01-08T22:10:04Z

hooks/hookexecution/execution_test.go

@@ -10,6 +10,12 @@ import (
 	"testing"
 )

+const (
+	testIpv6           = "1111:2222:3333:4444:5555:6666:7777:8888"
+	testIPv6Scubbed    = "1111:2222::"


Super nitpick: typo Scrubbed

bsardo · 2024-01-08T22:25:48Z

hooks/hookexecution/execution_test.go

@@ -137,13 +206,27 @@ func TestHandleModuleActivitiesNoBidderRequestPayload(t *testing.T) {
 			privacyConfig:       getTransmitUFPDActivityConfig("foo", true),
 			expectedPayloadData: hookstage.RawAuctionRequestPayload{},
 		},
+		{
+			description:         "payload should change when transmitPreciseGeo is blocked by activity",


Some of the test cases in this test have descriptions that indicate the payload should change but it doesn't.

Right, description fixed. Payload should NOT change in any of these cases, because hookstage.RawAuctionRequestPayload doesn't implement hookstage.RequestUpdater

bsardo · 2024-01-09T15:07:21Z

hooks/hookexecution/execution.go

+	if !transmitUserFPDActivityAllowed {
+		privacy.ScrubUserFPD(bidderReqCopy)
+	}
+	if !transmitPreciseGeoActivityAllowed {


In practice there should always be an account if we've gotten this far but perhaps we should add a check here to make sure the account is not nil just to be safe.

Correct, in practice we will always have an account because we call hookExecutor.SetAccount(account) in auction and amp_auction.
In order to add nil check for account, what do we expect to do if for some reason account is nil?

skip activity execution: privacy.ScrubGeoAndDeviceIP(bidderReqCopy, ipConf)

set default masking values(which also should be already set) and execute activity with default values:
account.Privacy.IPv6Config.AnonKeepBits = iputil.IPv6DefaultMaskingBitSize and
account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4DefaultMaskingBitSize

I suggest option 2 so that we still scrub given the sensitive nature.

bsardo · 2024-01-09T15:24:17Z

hooks/hookexecution/executor_test.go

@@ -699,7 +699,7 @@ func TestExecuteRawAuctionStage(t *testing.T) {

 func TestExecuteProcessedAuctionStage(t *testing.T) {
 	foobarModuleCtx := &moduleContexts{ctxs: map[string]hookstage.ModuleContext{"foobar": nil}}
-	account := &config.Account{}
+	account := &config.Account{Privacy: config.AccountPrivacy{}}


Nitpick: you shouldn't need to specify Privacy because you're defining a zero value account which will create a nested zero value Privacy object.

Right, privacy is not a pointer. Removed.

bsardo · 2024-01-09T15:30:41Z

hooks/hookexecution/executor_test.go

@@ -760,7 +760,7 @@ func TestExecuteProcessedAuctionStage(t *testing.T) {
 		{
 			description:            "Stage execution can be rejected - and later hooks rejected",
 			givenPlanBuilder:       TestRejectPlanBuilder{},
-			givenAccount:           nil,
+			givenAccount:           account,


If we add the account nil check in handleModuleActivities we shouldn't need to define an account here right? The way this test case is currently constructed, it seems like the only reason an account is being passed here now is to ensure PBS doesn't crash.

VeronikaSolovei9 · 2024-01-09T21:55:57Z

If we add the account nil check in handleModuleActivities we shouldn't need to define an account here right? The way this test case is currently constructed, it seems like the only reason an account is being passed here now is to ensure PBS doesn't crash.

Yes, it's needed for this test only. Do you think we may need to pass account with non-default values in future?
Currently in all test cases input account is just a not nil account:&config.Account{}.
We need to decide if we want to add a nil check for it in handleModuleActivities, I commented about possible cases in another comment above.

VeronikaSolovei9 · 2024-01-10T05:05:57Z

hooks/hookexecution/executor_test.go

@@ -699,7 +699,6 @@ func TestExecuteRawAuctionStage(t *testing.T) {

 func TestExecuteProcessedAuctionStage(t *testing.T) {
 	foobarModuleCtx := &moduleContexts{ctxs: map[string]hookstage.ModuleContext{"foobar": nil}}
-	account := &config.Account{}


Do we want to remove this from every test in this file?

I went through and tried removing all of these lines, and the tests still passed, so I think it can be removed.

I agree with Alex; we can remove them.

Added TransmitPreciseGeo activity handling in modules

484e500

bsardo assigned AlexBVolcy and bsardo Dec 13, 2023

AlexBVolcy reviewed Dec 14, 2023

View reviewed changes

VeronikaSolovei9 added 2 commits December 13, 2023 17:39

Code refactoring

eb06682

Code clean up

bdfec58

bsardo reviewed Dec 14, 2023

View reviewed changes

Code clean up

28396f8

VeronikaSolovei9 commented Dec 15, 2023

View reviewed changes

AlexBVolcy previously approved these changes Jan 4, 2024

View reviewed changes

bsardo reviewed Jan 9, 2024

View reviewed changes

Code clean up

d409cf5

VeronikaSolovei9 dismissed AlexBVolcy’s stale review via d409cf5 January 9, 2024 21:54

VeronikaSolovei9 added 2 commits January 9, 2024 13:56

Merge branch 'master' into modules_transmitPreciseGeo

7b67759

Code clean up

26557fe

VeronikaSolovei9 commented Jan 10, 2024

View reviewed changes

bsardo added the do not port label Jan 11, 2024

VeronikaSolovei9 added 2 commits January 11, 2024 13:00

Code clean up

f740479

Code clean up

1df492e

bsardo approved these changes Jan 16, 2024

View reviewed changes

AlexBVolcy approved these changes Jan 16, 2024

View reviewed changes

VeronikaSolovei9 merged commit 27b2ff6 into master Jan 16, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TransmitPreciseGeo activity handling in modules #3348

Added TransmitPreciseGeo activity handling in modules #3348

VeronikaSolovei9 commented Dec 12, 2023

AlexBVolcy left a comment

AlexBVolcy Dec 14, 2023

VeronikaSolovei9 Dec 14, 2023

bsardo Dec 14, 2023

VeronikaSolovei9 Dec 15, 2023

VeronikaSolovei9 Dec 15, 2023

bsardo Jan 9, 2024

bsardo Jan 8, 2024

bsardo Jan 8, 2024

VeronikaSolovei9 Jan 9, 2024

bsardo Jan 9, 2024

VeronikaSolovei9 Jan 9, 2024

bsardo Jan 10, 2024

VeronikaSolovei9 Jan 10, 2024

bsardo Jan 9, 2024

VeronikaSolovei9 Jan 9, 2024

bsardo Jan 9, 2024

VeronikaSolovei9 commented Jan 9, 2024 •

edited

Loading

VeronikaSolovei9 Jan 10, 2024

AlexBVolcy Jan 10, 2024

bsardo Jan 11, 2024

Added TransmitPreciseGeo activity handling in modules #3348

Added TransmitPreciseGeo activity handling in modules #3348

Conversation

VeronikaSolovei9 commented Dec 12, 2023

AlexBVolcy 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

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

VeronikaSolovei9 commented Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Jan 9, 2024 •

edited

Loading