Activities framework #2844

VeronikaSolovei9 · 2023-06-14T06:38:17Z

Implementation for #2591
Added foundation for activities evaluation and execution.

hhhjort · 2023-06-26T20:16:59Z

privacy/enforcer.go

+	}
+	for _, scope := range r.componentName {
+		if strings.EqualFold(scope.Scope, target.Scope) &&
+			(strings.EqualFold(scope.Name, target.Name) || scope.Name == "*") {


The compiler would hopefully optimize it anyway, but you could put the scope.Name == "*" first in the or since it is cheaper to compute than a longer string compare.

bsardo · 2023-06-27T19:15:54Z

exchange/utils.go

+			// skip the call to a bidder if fetchBids activity is not allowed
+			// do not add this bidder to allowedBidderRequests
+			continue
+		}


Notes from offline discussion on how to use activities framework along with existing privacy packages as part of GPP phase 3:

if fetchBidsActivityAllowed == privacy.ActivityDeny { // skip the call to a bidder if fetchBids activity is not allowed // do not add this bidder to allowedBidderRequests continue } else if fetchBidsActivityAllowed == privacy.ActivityAbstain and gdprEnforced == true { allowed, err := gdprPerms.FetchBidAllowed(ctx, bidderRequest.BidderCoreName, bidderRequest.BidderName) if !allowed { continue } } // proceed with processing ID and geo to see if needs to be scrubbed

bsardo · 2023-06-28T13:24:23Z

config/activity.go

@@ -8,16 +8,17 @@ type AllowActivities struct {
 	TransmitUserFPD          Activity `mapstructure:"transmitUfpd" json:"transmitUfpd"`
 	TransmitPreciseGeo       Activity `mapstructure:"transmitPreciseGeo" json:"transmitPreciseGeo"`
 	TransmitUniqueRequestIds Activity `mapstructure:"transmitUniqueRequestIds" json:"transmitUniqueRequestIds"`
+	TransmitTIds             Activity `mapstructure:"transmitTid" json:"transmitTid"`


Nitpick: Can we make the variable name match the name of the JSON field and follow go best practices on variable naming making this TransmitTID?

bsardo · 2023-06-28T13:31:07Z

config/account.go

@@ -40,7 +40,7 @@ type Account struct {
 	Validations             Validations                                 `mapstructure:"validations" json:"validations"`
 	DefaultBidLimit         int                                         `mapstructure:"default_bid_limit" json:"default_bid_limit"`
 	BidAdjustments          *openrtb_ext.ExtRequestPrebidBidAdjustments `mapstructure:"bidadjustments" json:"bidadjustments"`
-	Privacy                 AccountPrivacy                              `mapstructure:"privacy" json:"privacy"`
+	Privacy                 *AccountPrivacy                             `mapstructure:"privacy" json:"privacy"`


Why did we say this needed to be a pointer? I can't recall.

This was changed to "short-circuit NewActivityControl call in auction for performance reasons".
Also we need this nil check, at least for the meantime to make sure we don't reject all requests if activities are not configured.

bsardo · 2023-06-28T13:36:53Z

endpoints/openrtb2/amp_auction.go

@@ -239,6 +247,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
 		HookExecutor:               hookExecutor,
 		QueryParams:                r.URL.Query(),
 		TCF2Config:                 tcf2Config,
+		Activitities:               activities,


Typo: Activitities --> Activities

I'm sorry, renamed

bsardo · 2023-06-28T13:57:49Z

exchange/utils_test.go

+		privacyConfig := getDefaultActivityConfig(test.componentName, test.allow)
+		activities, err := privacy.NewActivityControl(privacyConfig)
+		assert.NoError(t, err, "")
+		auctionReq := AuctionRequest{
+			BidRequestWrapper: &openrtb_ext.RequestWrapper{BidRequest: test.req},
+			UserSyncs:         &emptyUsersync{},
+			Activitities:      activities,
+		}
+
+		bidderToSyncerKey := map[string]string{}
+		reqSplitter := &requestSplitter{
+			bidderToSyncerKey: bidderToSyncerKey,
+			me:                &metrics.MetricsEngineMock{},
+			hostSChainNode:    nil,
+			bidderInfo:        config.BidderInfos{},
+		}
+
+		bidderRequests, _, errs := reqSplitter.cleanOpenRTBRequests(context.Background(), auctionReq, nil, gdpr.SignalNo)
+		assert.Empty(t, errs, "unexpected error returned")
+		assert.Len(t, bidderRequests, test.expectedReqNumber, "incorrect number of requests")


I suggest wrapping this in a t.Run(test.name, func(t *testing.T) { and changing description to name and using underscores to separate the words in name.

You can also probably get rid of the string parameters at the end of each assert statement since the test framework should tell you what the problem is.

bsardo · 2023-06-28T15:25:25Z

privacy/enforcer.go

+}
+
+func NewActivityControl(privacyConf *config.AccountPrivacy) (ActivityControl, error) {
+	ac := ActivityControl{plans: nil}


Nitpick: isn't plans nil by default? You can probably omit that field in this instantiation.

If Activities are not configured, we need to return ActivityControl to avoid null pointer exception in future usage:

auctionReq.Activities.Allow(privacy.ActivityFetchBids, ...)

Plans should be set to nil, so it will handle undefined plan properly in Allow:

plan, planDefined := e.plans[activity] if !planDefined { return ActivityAbstain }

Sorry I just meant that this can probably just be ac := ActivityControl{} because nil is the default value for plans.

bsardo · 2023-06-28T19:07:24Z

privacy/enforcer_test.go

+			expectedResult:  ActivityAllow,
+		},
+		{
+			name:            "activityDefault is nil",


"activity_default_is_true"

bsardo · 2023-06-28T19:07:35Z

privacy/enforcer_test.go

+			expectedResult:  ActivityAllow,
+		},
+		{
+			name:            "activityDefault is nil",


"activity_default_is_false"

Sorry, I missed renaming these. Thank you.

bsardo · 2023-06-28T19:08:08Z

privacy/enforcer_test.go

+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			actualResult := activityDefaultToDefaultResult(test.activityDefault)
+			assert.Equal(t, actualResult, test.expectedResult, "result is incorrect")


Remove string and please reverse the actual and expected params to match the Equal signature.

bsardo · 2023-06-28T19:10:11Z

privacy/enforcer_test.go

+				ActivityFetchBids: getDefaultActivityPlan()}},
+			activity:       ActivityFetchBids,
+			target:         ScopedName{Scope: "bidder", Name: "bidderB"},
+			activityResult: ActivityAllow,


The name says it shouldn't be allowed but the result here is allow. I think deny is what we should expect here right?

In this case match is not found, but allowed by default value. I renamed it.

bsardo · 2023-06-28T19:10:45Z

privacy/enforcer_test.go

+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			actualResult := test.activityControl.Allow(test.activity, test.target)
+			assert.Equal(t, test.activityResult, actualResult, "incorrect allow activity result")


I think we can remove the string param.

…d account levels

bsardo · 2023-06-30T02:22:37Z

privacy/enforcer.go

+}
+
+func NewActivityControl(privacyConf *config.AccountPrivacy) (ActivityControl, error) {
+	ac := ActivityControl{plans: nil}


Sorry I just meant that this can probably just be ac := ActivityControl{} because nil is the default value for plans.

bsardo · 2023-06-30T02:44:08Z

privacy/enforcer.go

+			return result
+		}
+	}
+	return p.defaultResult


I do see this requirement in the issue and this is ultimately the behavior we want when activities is complete with privacy reg rules interspersed with component name/type rules and such. However, for now, I don't see how we can have a default value if privacy regs are meant to run after the activity framework as part of phase 3. Having a default that can only ever be allow or deny and then triggering privacy regs only if the activity framework abstains would mean the privacy regs would never run. I'll confirm the desired behavior here.

bsardo · 2023-06-30T02:52:12Z

privacy/enforcer.go

+		return ac, err
+	} else {
+		//temporarily disable Activities if they are specified at the account level
+		return ac, errors.New("account.Privacy has no effect as the feature is under development.")


Can we easily make this a warning reported back to the publisher so the request is still processed?

Yes, modified

bsardo · 2023-06-30T02:55:24Z

config/config.go

@@ -155,6 +155,10 @@ func (cfg *Configuration) validate(v *viper.Viper) []error {
 		cfg.TmaxAdjustments.Enabled = false
 	}

+	if cfg.AccountDefaults.Privacy != nil {
+		errs = append(errs, errors.New("account_defaults.Privacy has no effect as the feature is under development."))


I think maybe using glog.Warning instead of recording an error might be better here.

I changed this to glog.Warning and there are no messages at the server startup. Instead I always see warning message in response debug logs, even if account config is correct.
I think it's ok for the meantime while this feature is disabled. Eventually both warnings will go away.

bsardo · 2023-06-30T03:03:54Z

privacy/enforcer.go

+	if len(r.componentName) == 0 {
+		componentNameFound = true
+	}


I do see that the most recent change here works. However, I think my issue is more with the fact that we have two variable names componentNameFound and componentTypeFound that are sometimes set to true when in fact nothing has been found. I think for clarity we should find another way of representing when componentName and componentType clauses have been omitted from the rule which is why I suggested two additional variables with names that indicate presence.
It does mean that the matchFound statement is more verbose but I think it is more obvious what constitutes a match:
matchFound := (nameFound || !nameClauseExists) && (typeFound || !typeClauseExists)

We don't have to use this exact solution; it's just one way to address my concern.

hhhjort

LGTM

@akkapur

* Fix: deal tiers no longer ignored due to presence of tid (prebid#2829) * CAPT-787: GPP support for imds bidder. (prebid#2867) Co-authored-by: Timothy M. Ace <tace@imds.tv> * Adsinteractive: change usersync endpoint to https (prebid#2861) Co-authored-by: Balint Vargha <varghabalint@gmail.com> * consumable adapter: add gpp support (prebid#2883) * feat: IX Bid Adapter - gpp support for user sync urls (prebid#2873) Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> * fix: update links in readme (prebid#2888) authored by @akkapur * New Adapter: AIDEM (prebid#2824) Co-authored-by: AndreaC <67786179+darkstarac@users.noreply.github.com> Co-authored-by: Andrea Tumbarello <Wazabit@users.noreply.github.com> Co-authored-by: darkstar <canazza@wazabit.it> * Improve Digital adapter: Set currency in bid response (prebid#2886) * Sharethrough: Support multiformat bid request impression (prebid#2866) * Triplelift Bid Adapter: Adding GPP Support (prebid#2887) * YahooAdvertising rebranding to Yahoo Ads. (prebid#2872) Co-authored-by: oath-jac <dsp-supply-prebid@verizonmedia.com> * IX: MultiImp Implementation (prebid#2779) Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> Co-authored-by: Oronno Mamun <oronno.mamun@indexexchange.com> * Exchange unit test fix (prebid#2868) * Semgrep rules for adapters (prebid#2833) * IX: Remove glog statement (prebid#2909) * Activities framework (prebid#2844) * PWBID: Update Default Endpoint (prebid#2903) * script to run semgrep tests against adapter PRs (prebid#2907) authored by @onkarvhanumante * semgrep rule to detect undesirable package imports in adapter code (prebid#2911) * update package-import message (prebid#2913) authored by @onkarvhanumante * Bump google.golang.org/grpc from 1.46.2 to 1.53.0 (prebid#2905) --------- Co-authored-by: Brian Sardo <1168933+bsardo@users.noreply.github.com> Co-authored-by: Timothy Ace <github.com-1@timothyace.com> Co-authored-by: Timothy M. Ace <tace@imds.tv> Co-authored-by: balintvargha <122350182+balintvargha@users.noreply.github.com> Co-authored-by: Balint Vargha <varghabalint@gmail.com> Co-authored-by: Jason Piros <jasonpiros@gmail.com> Co-authored-by: ccorbo <ccorbo2013@gmail.com> Co-authored-by: Chris Corbo <chris.corbo@indexexchange.com> Co-authored-by: Ankush <ankush.kapur@gmail.com> Co-authored-by: Giovanni Sollazzo <gs@aidem.com> Co-authored-by: AndreaC <67786179+darkstarac@users.noreply.github.com> Co-authored-by: Andrea Tumbarello <Wazabit@users.noreply.github.com> Co-authored-by: darkstar <canazza@wazabit.it> Co-authored-by: Jozef Bartek <31618107+jbartek25@users.noreply.github.com> Co-authored-by: Max Dupuis <118775839+maxime-dupuis@users.noreply.github.com> Co-authored-by: Patrick Loughrey <ploughrey@triplelift.com> Co-authored-by: radubarbos <raduiquest79@gmail.com> Co-authored-by: oath-jac <dsp-supply-prebid@verizonmedia.com> Co-authored-by: Oronno Mamun <oronno.mamun@indexexchange.com> Co-authored-by: Veronika Solovei <kalypsonika@gmail.com> Co-authored-by: Onkar Hanumante <onkar.hanumante@xandr.com> Co-authored-by: Stephen Johnston <tiquortoo@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

VeronikaSolovei9 added 2 commits June 13, 2023 23:36

Initial activities framework

90278ba

Fix for condition evaluation

fce25c0

bsardo self-assigned this Jun 15, 2023

VeronikaSolovei9 added 7 commits June 15, 2023 18:12

Rules evaluation logic fixes

d2c5c9d

No merge for host and account configs needed at the activities level

63c4d78

Added activities support ror am endpoint

b60143d

Added fetchBids activity support

d6af286

Code clean up

d5f13d7

Unit tests part I

c0457b8

Unit tests part II

62fdcb6

VeronikaSolovei9 marked this pull request as ready for review June 24, 2023 03:59

hhhjort self-assigned this Jun 26, 2023

hhhjort reviewed Jun 26, 2023

View reviewed changes

bsardo reviewed Jun 27, 2023

View reviewed changes

Minor optimization

fb353b2

bsardo reviewed Jun 28, 2023

View reviewed changes

VeronikaSolovei9 added 3 commits June 28, 2023 15:28

Code review fixes

7bc6027

Added conditional gdpr execution if activity result is abstain

40e6491

Added extra verification activities are not configured at the host an…

99c21ea

…d account levels

bsardo reviewed Jun 30, 2023

View reviewed changes

Minor tweaks

5983391

bsardo approved these changes Jul 1, 2023

View reviewed changes

hhhjort approved these changes Jul 5, 2023

View reviewed changes

bsardo merged commit 23fffa1 into master Jul 6, 2023

cross-repo-issue-creation bot mentioned this pull request Jul 6, 2023

Port PR from PBS-Go: Activities framework prebid/prebid-server-java#2487

Closed

Peiling-Ding pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jul 6, 2023

Activities framework (prebid#2844)

5cb84ef

Peiling-Ding pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jul 14, 2023

Activities framework (prebid#2844)

42a10d1

onkarvhanumante mentioned this pull request Jul 27, 2023

fix go-vet errors related to privacy/enforcer #2978

Closed

SyntaxNode deleted the activities branch October 2, 2023 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activities framework #2844

Activities framework #2844

VeronikaSolovei9 commented Jun 14, 2023

hhhjort Jun 26, 2023

bsardo Jun 27, 2023 •

edited

Loading

bsardo Jun 28, 2023

bsardo Jun 28, 2023

VeronikaSolovei9 Jun 28, 2023

bsardo Jun 28, 2023

VeronikaSolovei9 Jun 28, 2023

bsardo Jun 28, 2023

bsardo Jun 28, 2023

VeronikaSolovei9 Jun 28, 2023

bsardo Jun 30, 2023

bsardo Jun 28, 2023

bsardo Jun 28, 2023

VeronikaSolovei9 Jun 28, 2023

bsardo Jun 28, 2023

bsardo Jun 28, 2023

VeronikaSolovei9 Jun 28, 2023

bsardo Jun 28, 2023

bsardo Jun 30, 2023

bsardo Jun 30, 2023 •

edited

Loading

bsardo Jun 30, 2023

VeronikaSolovei9 Jun 30, 2023

bsardo Jun 30, 2023

VeronikaSolovei9 Jun 30, 2023

bsardo Jun 30, 2023

hhhjort left a comment

Activities framework #2844

Activities framework #2844

Conversation

VeronikaSolovei9 commented Jun 14, 2023

Choose a reason for hiding this comment

bsardo Jun 27, 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

bsardo Jun 30, 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

hhhjort left a comment

Choose a reason for hiding this comment

bsardo Jun 27, 2023 •

edited

Loading

bsardo Jun 30, 2023 •

edited

Loading