Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Privacy Activity Config #2740

Merged
merged 2 commits into from
May 18, 2023
Merged

Privacy Activity Config #2740

merged 2 commits into from
May 18, 2023

Conversation

SyntaxNode
Copy link
Contributor

Implements configuration for GPP Phase 3 privacy activities.

Copy link
Contributor Author

@SyntaxNode SyntaxNode May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the config or privacy package? I think config is the best choice. Though this is the same data model for the rules engine and will likely be used verbatim. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. The actual configuration belongs here. Data structures and methods to actually carry out the frame work can live in privacy.

ActivityTransmitUniqueRequestIds
)

func (a Activity) String() string {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String form is needed for metrics, logs, debugging, etc.. Defining as an integer for smaller internal representation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more efficient to define the strings in an array rather than case structure? You don't anticipate unused numbers, do you?

guscarreon
guscarreon previously approved these changes May 3, 2023
Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me. I just have one naming question.

@@ -39,6 +39,7 @@ type Account struct {
PriceFloors AccountPriceFloors `mapstructure:"price_floors" json:"price_floors"`
Validations Validations `mapstructure:"validations" json:"validations"`
DefaultBidLimit int `mapstructure:"default_bid_limit" json:"default_bid_limit"`
Privacy AccountPrivacy `mapstructure:"privacy" json:"privacy"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GPP Phase 4 allowactivities seems to be inside a consent field. Should we rename to consent instead?

 40       Validations             Validations                          `mapstructure:"validations" json:"validations"`
 41       DefaultBidLimit         int                                  `mapstructure:"default_bid_limit" json:"default_bid_limit"`
 42 -     Privacy                 AccountPrivacy                       `mapstructure:"privacy" json:"privacy"`
    +     Consent                 AccountPrivacyConsent                `mapstructure:"consent" json:"consent"`
 43   }
 config/account.go

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Phase 4 docs are outdated. We changed it to "privacy" in the Phase 3 doc. I'm sure Phase 4 will be updated soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in the referenced issue.

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Condition ActivityCondition `mapstructure:"condition" json:"condition"`
}

type ActivityCondition struct {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these will need to be fleshed out a little further when we actually implement the activity infrastructure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is everything needed for Phase 3, right? In Phase 4 there is also planned for privacyreg, but let's finish out Phase 3 first. The implementation for Phase 3 isn't started yet. To keep PRs small, this is just the config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phase 3 defines namespace and wildcards, but those are details of the string value parsing and are not represented at this level. Perhaps they could be though? Did you intend to suggest adding a "name scoped value" of some sort with custom unmarshal/marshal methods to handle the parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is Viper and not standard json. Probably best to leave the "named scope" parsing for other parts of code so we don't have too much Viper specialized functionality.

hhhjort
hhhjort previously approved these changes May 8, 2023
Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@SyntaxNode SyntaxNode dismissed stale reviews from hhhjort and guscarreon via 187081f May 17, 2023 16:43
Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hhhjort hhhjort merged commit 324d293 into prebid:master May 18, 2023
blueseasx pushed a commit to blueseasx/prebid-server that referenced this pull request Jun 2, 2023
@SyntaxNode SyntaxNode deleted the activities-1 branch July 10, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants