Modules activities #3057

VeronikaSolovei9 · 2023-08-25T20:09:40Z

Added activities support in modules. Only one activity is now added: ActivityTransmitUserFPD
Activity evaluation is running before each module hook invocation. If data transmission is prohibited by activity - only module hook payload will be modified.

SyntaxNode · 2023-09-05T06:51:45Z

hooks/hookexecution/execution.go

+
+func handleModuleActivities[T any, P any](hook hooks.HookWrapper[T], activityControl privacy.ActivityControl, payload P) P {
+	// only 2 stages receive bidder request: hookstage.ProcessedAuctionRequestPayload and hookstage.BidderRequestPayload
+	// they both implement PayloadBidderRequest interface in order to execute mutations on bid request


IMHO - This comment isn't necessary.

SyntaxNode · 2023-09-05T06:53:11Z

hooks/hookexecution/execution.go

+	// only 2 stages receive bidder request: hookstage.ProcessedAuctionRequestPayload and hookstage.BidderRequestPayload
+	// they both implement PayloadBidderRequest interface in order to execute mutations on bid request
+	var payloadData hookstage.PayloadBidderRequest
+	ok := true


Nitpick: Might look a bit nicer to group the definitions.. or maybe not,. This one may be subjective. The ok := true just looks a bit odd to me (although I understand why it's done).

payloadData, ok := any(&payload).(hookstage.PayloadBidderRequest) !ok { // payload doesn't have a bid request return payload }

SyntaxNode · 2023-09-05T06:55:09Z

hooks/hookexecution/execution.go

+	privacyEnforcement := privacy.Enforcement{}
+
+	// parse hook.Module to split it to type and mame?
+	// hook.Module example: "mytest.mymodule". Can it be "rtd.mymodule" or "general.mymodule"?


We need more information from the module to make this determination. Consider adding an RTD flag? .. or a Kind type of flag?

Do you mean we need to add new property to type HookWrapper[T any] struct? Like IsRTD bool?

SyntaxNode · 2023-09-05T06:55:58Z

hooks/hookexecution/execution.go

+	privacyEnforcement := privacy.Enforcement{}
+
+	// parse hook.Module to split it to type and mame?
+	// hook.Module example: "mytest.mymodule". Can it be "rtd.mymodule" or "general.mymodule"?


I think we use mytest_mymodule as the full module's name in other places.

I'm confused here.
This is how hooks configuration looks like:

"hook_sequence": [ { "module_code": "mytest.mymodule", "hook_impl_code": "mymodule" }, { "module_code": "mytest2.mymodule2", "hook_impl_code": "mymodule2" } ]

this is how activity configuration looks like:

"rules": [ { "allow": false, "condition": { "componentName": [ "general.mymodule2" ] } } ]

Do we expect to have activity component name set to rtd.mytest.mymodule or general.mytest.mymodule?
It will need an adjustment for activity rules parser, becaule we only expect to have one . in componentName.

My thought is the mytest part is the same thing as general and rtd. It is just we only envision those two categories as being relevant at present. Do you have a different understanding of where the mytest piece of the name is coming from?

Yes, this what I was thinking too. The problem here is that these two modules are created and configured by me and I can give any name to the directory where the module will be. Modules should be placed in {PBS_root}/modules directory.

In order to make activities work this directories can only have names rtd and general.

Maybe we should set module type to hook_impl_code, like

{ "module_code": "mytest.mymodule", "hook_impl_code": "rtd.mymodule" }

SyntaxNode · 2023-09-05T06:56:23Z

hooks/hookexecution/execution.go

+		// changes need to be applied to new payload and leave original payload unchanged
+		bidderReq := payloadData.GetBidderRequestPayload()
+		var bidderReqCopy *openrtb2.BidRequest
+		bidderReqCopy = ptrutil.Clone(bidderReq)


Nitpick: bidderReqCopy := ptrutil.Clone(bidderReq)

Nevermind. We discovered this breaks IntelliJ auto-complete.

SyntaxNode · 2023-09-05T06:57:50Z

hooks/hookexecution/execution.go

+		var newPayload = payload
+		var np = any(&newPayload).(hookstage.PayloadBidderRequest)
+		np.SetBidderRequestPayload(bidderReqCopy)
+		return newPayload


What's the point of newPayload? Does it make a shallow copy?

At this point we know already that request will be changed for this module (for instance user.ID will be removed).
We create a shallow copy before "Apply" and inside of the "Apply" new User will be created and set back to request so this module will not receive restricted data. We don't do this if activity will not change the request.

if privacyEnforcement.AnyActivities() { // changes need to be applied to new payload and leave original payload unchanged bidderReq := payloadData.GetBidderRequestPayload() bidderReqCopy := ptrutil.Clone(bidderReq) privacyEnforcement.Apply(bidderReqCopy) var newPayload = payload var np = any(&newPayload).(hookstage.PayloadBidderRequest) np.SetBidderRequestPayload(bidderReqCopy) return newPayload }

So you have a shallow copy of the bidrequest. That shallow copy includes a pointer to the original User object. If you remove the ID from the User object, you will be modifying the original request.

What you are saying is correct. In this case User copy will be handled inside of Apply function. It will execute ScrubRequest function and it will make copy of User at the beginning and then set it back to request

var userCopy *openrtb2.User userCopy = ptrutil.Clone(bidRequest.User) ... ... bidRequest.User = userCopy return bidRequest

hhhjort · 2023-09-06T20:01:00Z

hooks/hookexecution/execution_test.go

+		})
+	}
+}
+


Perhaps a test to insure the original request remains unmolested after the payload has been modified for the module?

Yes, this is what is checked in hooks/hookexecution/executor_test.go where tests are added for the functions ExecuteRawAuctionStage and ExecuteBidderRequestStage. The only difference from other tests there is that Activities with the restricted userFPD option is passed.

SyntaxNode · 2023-10-23T01:33:40Z

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.

# Conflicts: # hooks/hookexecution/context.go # hooks/hookexecution/execution.go # hooks/hookexecution/executor.go # hooks/hookexecution/executor_test.go

AlexBVolcy · 2023-11-27T21:38:25Z

hooks/hookexecution/execution.go

+		return payload
+	}
+
+	// parse hook.Module to split it to type and mame?


Does this comment need to be updated? The question mark at the end suggests this to me. And also there's a slight typo mame --> name

This comment is not needed, removed

AlexBVolcy · 2023-11-27T21:44:25Z

hooks/hookstage/bidderrequest.go

+}
+
+// PayloadBidderRequest indicated of hook carries a bid request.
+// used for activities, name can be better


Just wanted to point out another comment that might need to be updated, can we remove name can be better?

AlexBVolcy · 2023-11-27T21:47:51Z

ortb/clone.go

+	var newReq *openrtb2.BidRequest
+	newReq = ptrutil.Clone(req)


I think these lines can be consolidated to just this:

newReq := ptrutil.Clone(req)

Tests still passed when I updated this locally.

Yes! I recently updated my IntelliJ to the newest version and this feature works and doesn't "yell" at me about generic type!

AlexBVolcy · 2023-11-27T21:50:54Z

hooks/hookexecution/context.go

-	moduleContexts *moduleContexts
+	endpoint        string
+	stage           string
+	accountId       string


Nitpick: I think there's a go convention to have Id be capitalized, so it'd look like accountID

bsardo · 2023-12-04T17:49:32Z

hooks/hookstage/bidderrequest.go

+	brp.Request = br
+}
+
+// PayloadBidderRequest indicated of hook carries a bid request.


Nitpick: Can we reword this a bit differently? I'm not sure what it's trying to convey.

Yes, basically I want to say this interface exists to indicate BidRequest is present in payload. Now we have 2 implementations: BidderRequestPayload and ProcessedAuctionRequestPayload
What do you think a good comment will be?

I would probably just say something like // RequestUpdater allows reading and writing a bid request. See my other comment about renaming this interface.

bsardo · 2023-12-04T17:51:23Z

hooks/hookstage/bidderrequest.go

+func (brp *BidderRequestPayload) GetBidderRequestPayload() *openrtb_ext.RequestWrapper {
+	return brp.Request
+}
+
+func (brp *BidderRequestPayload) SetBidderRequestPayload(br *openrtb_ext.RequestWrapper) {
+	brp.Request = br
+}


I'm curious, do we need a get and set? Could we access the request directly given that the payload is public?

Yes, we need a unified interface to get and set RequestWraper from payloads where we have it. We also need this interface to check data type in handleModuleActivities, we only process activities if input type has a BidderRequest in payload:

payloadData, ok := any(&payload).(hookstage.PayloadBidderRequest) if !ok { return payload }

With this interface we don't need to know the exact type of Payload, we only need to know if it has a BidderRequest.

Gotcha, I see the need for the interface. I got a little confused by the name of the interface when looking at the code in handleModuleActivities. I originally thought the name PayloadBidderRequest in that function was referring to a specific type of payload, BidderRequestPayload, rather than an interface. I suggest we change the name of this interface to something like RequestMutator, RequestReaderWriter, RequestWriter, RequestModifier, RequestUpdater, RequestReplacer etc. to reduce confusion and to have the interface name avoid constraining us to a specific underlying data type.

renamed to RequestUpdater

bsardo · 2023-12-04T17:52:25Z

hooks/hookstage/processedauctionrequest.go

+func (parp *ProcessedAuctionRequestPayload) GetBidderRequestPayload() *openrtb_ext.RequestWrapper {
+	return parp.Request
+}
+
+func (parp *ProcessedAuctionRequestPayload) SetBidderRequestPayload(br *openrtb_ext.RequestWrapper) {
+	parp.Request = br
+}


Same comment as in bidderrequest.go. Why add a get and set that just read back the value or do a simple assignment?

Answered for BidderRequestPayload

bsardo · 2023-12-05T04:03:07Z

hooks/hookexecution/execution.go

@@ -311,3 +314,29 @@ func handleHookMutations[P any](

 	return payload
 }
+
+func handleModuleActivities[T any, P any](hook hooks.HookWrapper[T], activityControl privacy.ActivityControl, payload P) P {


Are generics needed here at this time? It looks like we only care about the bidder request hook at the moment. I'm also not sure if generics will work to support additional hooks without more work given that this function always casts the payload to the bidder request payload instead of deriving the payload type somehow from the T and/or P. Thoughts?

We need to know hook hooks.HookWrapper[T] and payload P to process activities.
P can be any of hookstage payload: AllProcessedBidResponsesPayload, AuctionResponsePayload, RawBidderResponsePayload and others.
We only need to check and run Activities for implementations of PayloadBidderRequest

Or I don't understand your question.

It looks like this function signature can be simplified a bit to exclude type parameter T and instead just pass in hookCode string instead of hook:
func handleModuleActivities[P any](hookCode string, activityControl privacy.ActivityControl, payload P) P {

In general, I'm still on the fence here on whether generics make sense, mostly because this function is basically a noop for all payloads that don't include a request. In that case, I thought interfaces were a better fit where P becomes whatever your interface name is (e.g. RequestModifier) but that would require the calling code to perform the type analysis on P and converting it to the interface type which may not make such a change worth it. Given that this code is called by generic code there really isn't any getting away from the type analysis. The only potential benefit I see to such a change, ignoring any potential performance implications, is that this function is better scoped. We can discuss more offline.

hook hooks.HookWrapper[T] - removed!
I tried to move the P check outside, but don't think it fits there. It's related to activitites only.
We still need to have a payload(payloadData) casted to hookstage.RequestUpdater later in the func handleModuleActivities.

Can we keep it inside of the handleModuleActivities and don't overload executeGroup?

bsardo · 2023-12-05T04:04:52Z

endpoints/openrtb2/auction.go

@@ -205,6 +205,8 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http

 	activityControl = privacy.NewActivityControl(&account.Privacy)

+	hookExecutor.SetActivityControl(activityControl)


Do we need to call this for other endpoints as well?

Ooof, yes, I will add this to amp and video.

Added to AMP endpoint only.
In Video endpoint hooks are not configured:
HookExecutor: hookexecution.EmptyHookExecutor{},

bsardo · 2023-12-05T04:06:57Z

ortb/clone_test.go

+			Source: &openrtb2.Source{TID: "testTID"},
+		}
+		result := CloneBidderReq(given)
+		assert.NotSame(t, given, result.BidRequest, "pointer")


Do we want an assert.Equal here to ensure the values are the same?

Pointers should not be the same. We clone BidRequest too: newReq := ptrutil.Clone(req) and then User, Device and Source.

Yes but the values should be the same right? I think assert.Equal is a value comparison rather than a memory address comparison.

Oh yes! Values should be the same, but pointers different. Do you want me to add assert.Equal for values check?

bsardo · 2023-12-05T18:58:00Z

hooks/hookexecution/executor_test.go

@@ -669,13 +672,76 @@ func TestExecuteRawAuctionStage(t *testing.T) {
 				},
 			},
 		},
+		{


If I understand correctly, this test should not modify the payload because it involves the raw auction hook instead of the bidder request hook right? The raw auction hook payload is different from the one used in the bidder request hook and does not currently support activities.

This test is probably not needed. Idea here is to test that for the stages and payloads without BidderRequest activities do nothing and don't modify anything. Inn this case we execute ExecuteRawAuctionStage with RawAuctionRequestPayload and with disabled activity and verify nothing was changed. This test exercises scenario where "ok" is false.

payloadData, ok := any(&payload).(hookstage.PayloadBidderRequest) if !ok { return payload }

Gotcha, yeah I think you can probably get rid of this test. You have appropriate test coverage of the false case in execution_test.go#TestHandleModuleActivitiesNoBidderRequestPayload.

As for making sure the original payload isn't modified by handleModuleActivities, I think you can easily ensure that by updating the three tests in execution_test.go with the following if you'd like:

origInPayloadData := test.inPayloadData assert.Equal(t, origInPayloadData, test.inPayloadData)

bsardo · 2023-12-05T19:00:38Z

hooks/hookexecution/executor_test.go

@@ -1164,12 +1231,68 @@ func TestExecuteBidderRequestStage(t *testing.T) {
 				},
 			},
 		},
+		{
+			description:           "Group payload not changes with transmitUFPD activity restriction",


Why isn't this test modifying the payload? This involves the bidder request hook, whose payload has activity support, and you've set it up so the activity infrastructure says transmitting FPD is not allowed. Shouldn't the payload be modified?

Is the issue the fact that this test has always compared givenBidderRequest with expectedBidderRequest expecting them to be equal, which should still be true even though we are cloning the the bidder request portion of the payload and modifying it?

In this test we check that input BidderRequest didn't change and remains the same.
In this test we execute ExecuteBidderRequestStage with 2 groups (each with one hook). We modify BidRequest for first hook, because it matches the restriction config and don't modify BidderRequest for the second group.
Modified payload (BidRequest) is only passed into executeHook.

newPayload := handleModuleActivities(hook, executionCtx.activityControl, payload) wg.Add(1) go func(hw hooks.HookWrapper[H], moduleCtx hookstage.ModuleInvocationContext) { defer wg.Done() executeHook(moduleCtx, hw, newPayload, hookHandler, group.Timeout, resp, rejected) }(hook, mCtx)

Incoming BidRequest remains the same.

In other words: BidRequest outside of func executeGroup[H any, P any]( should remain unchanged.

I find this test to be a little redundant and confusing, particularly because it has the exact same inputs and outputs as other test cases except for the fact that there is an activities object input.

What do you think about deleting this test case and instead adding this code block to each of the six stage tests in this file right after the call to SetAccount:

privacyConfig := getTransmitUFPDActivityConfig("foo", false) ac := privacy.NewActivityControl(privacyConfig) exec.SetActivityControl(ac)

In this case, we will be taking advantage of the test cases that already exist throughout the file, exercising the activity infrastructure logic for each stage (not a necessity IMO given the nice unit test coverage you have in execution_test.go but comes for free here), and introducing some symmetry which I think makes it easier for us to maintain this as it makes the coverage intent easier to discover. On top of that, what you're trying to test here will be exercised for each test case because the TestApplyHookMutationsBuilder used in each of the six stage tests already defines a foo hook in each stage plan.
Thoughts?

Agree, modified!

bsardo · 2023-12-05T19:42:55Z

hooks/hookexecution/execution_test.go

+	}
+}
+
+func TestHandleModuleActivitiesProcessedAuctionRequestPayload(t *testing.T) {


It looks like processed auction request works because the payload type is able to be casted to the bidder request payload type, is that correct?
Are these the only two payload types we care about because they both deal with a bid request?

Correct, ProcessedAuctionRequestPayload and BidderRequestPayload are only payloads with BidderRequest and they both implement PayloadBidderRequest interface.
This makes them to pass type check here payloadData, ok := any(&payload).(hookstage.PayloadBidderRequest)

bsardo · 2023-12-11T14:19:57Z

hooks/hookexecution/execution_test.go

+			//check input payload didn't change
+			origInPayloadData := test.inPayloadData
+			assert.Equal(t, origInPayloadData, test.inPayloadData)


I think you need to move the line origInPayloadData := test.inPayloadData above the call to handleModuleActivities.

bsardo · 2023-12-11T14:22:44Z

hooks/hookexecution/execution_test.go

+			//check input payload didn't change
+			origInPayloadData := test.inPayloadData
+			assert.Equal(t, origInPayloadData, test.inPayloadData)


I think you need to move the line origInPayloadData := test.inPayloadData above the call to handleModuleActivities.

bsardo · 2023-12-11T14:22:49Z

hooks/hookexecution/execution_test.go

+			//check input payload didn't change
+			origInPayloadData := test.inPayloadData
+			assert.Equal(t, origInPayloadData, test.inPayloadData)


I think you need to move the line origInPayloadData := test.inPayloadData above the call to handleModuleActivities.

bsardo · 2023-12-11T14:24:33Z

hooks/hookexecution/executor_test.go

+			privacyConfig := getTransmitUFPDActivityConfig("foo", false)
+			ac := privacy.NewActivityControl(privacyConfig)
 			exec.SetActivityControl(ac)
+			exec.SetAccount(test.givenAccount)


Are you able to add this to all six stage tests?

TestExecuteRawAuctionStage

TestExecuteProcessedAuctionStage

TestExecuteBidderRequestStage

TestExecuteRawBidderResponseStage

TestExecuteAllProcessedBidResponsesStage

TestExecuteAuctionResponseStage

VeronikaSolovei9 added 4 commits August 25, 2023 11:02

Initial version

8bdbbae

Replaced to generics

89509aa

Minor refactoring

03340ba

Working end to end with multiple modules: some restricted and some not

a0c2acb

SyntaxNode self-requested a review August 28, 2023 17:15

SyntaxNode self-assigned this Aug 28, 2023

VeronikaSolovei9 added 3 commits August 28, 2023 11:42

Refactored duplicated code

0171679

Changed mutation functions to scrubber

e7e03cb

Remover unnecessary error

97b2639

hhhjort self-requested a review August 31, 2023 17:24

hhhjort self-assigned this Aug 31, 2023

Unit tests

d0de9a9

VeronikaSolovei9 marked this pull request as ready for review September 2, 2023 05:14

Imports sort

c830408

SyntaxNode reviewed Sep 5, 2023

View reviewed changes

SyntaxNode added the do not port label Sep 5, 2023

VeronikaSolovei9 added 3 commits September 5, 2023 13:59

Code clean up

2d9dfc9

Merge branch 'master' into modules_activities

0c4a4db

Merge fixes

a026019

hhhjort reviewed Sep 6, 2023

View reviewed changes

Merge branch 'master' into modules_activities

f2702ae

bsardo added the blocked label Sep 21, 2023

VeronikaSolovei9 added 2 commits November 16, 2023 15:20

Merge branch 'master' into modules_activities

90c4f20

# Conflicts: # hooks/hookexecution/context.go # hooks/hookexecution/execution.go # hooks/hookexecution/executor.go # hooks/hookexecution/executor_test.go

Merge with master and fixes

d019263

VeronikaSolovei9 removed the blocked label Nov 17, 2023

bsardo assigned AlexBVolcy and unassigned hhhjort Nov 20, 2023

bsardo assigned bsardo and unassigned SyntaxNode Nov 27, 2023

AlexBVolcy reviewed Nov 27, 2023

View reviewed changes

VeronikaSolovei9 added 2 commits November 28, 2023 10:34

Merge branch 'master' into modules_activities

30ca359

Code clean up

e67b1d9

AlexBVolcy previously approved these changes Nov 29, 2023

View reviewed changes

VeronikaSolovei9 mentioned this pull request Nov 30, 2023

Use TransmitUserFPD and TransmitPreciseGeo activities in analytics #3331

Merged

bsardo reviewed Dec 5, 2023

View reviewed changes

Interface rename, code clean up

9b71cd2

VeronikaSolovei9 dismissed AlexBVolcy’s stale review via 9b71cd2 December 7, 2023 03:24

VeronikaSolovei9 added 2 commits December 6, 2023 19:26

Merge branch 'master' into modules_activities

bf08739

Unit tests clean up

921a52d

bsardo reviewed Dec 11, 2023

View reviewed changes

Unit tests clean up

fb6be8a

AlexBVolcy approved these changes Dec 11, 2023

View reviewed changes

bsardo approved these changes Dec 11, 2023

View reviewed changes

bsardo merged commit ec873dc into master Dec 11, 2023

SyntaxNode deleted the modules_activities branch December 19, 2023 17:10

		@@ -205,6 +205,8 @@ func (deps endpointDeps) Auction(w http.ResponseWriter, r http.Request, _ http

		activityControl = privacy.NewActivityControl(&account.Privacy)

		hookExecutor.SetActivityControl(activityControl)

@@ @@ -669,13 +672,76 @@ func TestExecuteRawAuctionStage(t *testing.T) { @@
               				},
               			},
               		},
+              		{

Modules activities #3057

Modules activities #3057

Conversation

VeronikaSolovei9 commented Aug 25, 2023 • 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

SyntaxNode commented Oct 23, 2023

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 Dec 4, 2023 • 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

bsardo Dec 6, 2023 • 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

bsardo Dec 6, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Aug 25, 2023 •

edited

Loading

bsardo Dec 4, 2023 •

edited

Loading

bsardo Dec 6, 2023 •

edited

Loading

bsardo Dec 6, 2023 •

edited

Loading