Metrics: Add per adapter metric indicating when buyeruid was scrubbed #3623
Conversation
| @@ -560,6 +560,9 @@ type DisabledMetrics struct { | |||
| // that were created or reused. | |||
| AdapterConnectionMetrics bool `mapstructure:"adapter_connections_metrics"` | |||
|
|
|||
| // True if we don't want to collect the per adapter buyer UID scrubbed metric | |||
| AdapterBuyerUIDScrubbed bool `mapstructure:"adapter_buyeruid_scrubbed"` | |||
There was a problem hiding this comment.
Could you add this field to config_test.go, with TestDefaults and TestFullConfig?
exchange/utils.go
Outdated
| @@ -202,6 +205,9 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context, | |||
| privacy.ScrubDeviceIDsIPsUserDemoExt(reqWrapper, ipConf, "eids", false) | |||
| } | |||
| } | |||
| if buyerUIDSet && reqWrapper.User.BuyerUID == "" { | |||
There was a problem hiding this comment.
I was thinking, could this be re-written, removing the lines 191-194, and instead just put the conditions here:
if reqWrapper.User != nil && reqWrapper.User.BuyerUID != "" && reqWrapper.User.BuyerUID == "" {
rs.me.RecordAdapterBuyerUIDScrubbed(bidderRequest.BidderCoreName)
}
What do you think about this?
There was a problem hiding this comment.
Ah if you look at your suggested snippet again you'll see that you're referencing reqWrapper.User.BuyerUID twice. The reason I wrote it the way I did is so I can capture reqWrapper.User.BuyerUID as buyerUIDSet before it is potentially overwritten by the scrubber logic.
I am then checking if the value was set and has since been cleared by the scrubber before logging the metric.
Does that make sense?
metrics/go_metrics_test.go
Outdated
| description: "", | ||
| metricsDisabled: false, | ||
| adapterName: openrtb_ext.BidderName(adapter), | ||
| expectedCount: 1, | ||
| }, | ||
| { | ||
| description: "", | ||
| metricsDisabled: false, | ||
| adapterName: fakeBidder, | ||
| expectedCount: 0, | ||
| }, | ||
| { | ||
| description: "", | ||
| metricsDisabled: true, | ||
| adapterName: openrtb_ext.BidderName(adapter), | ||
| expectedCount: 0, |
There was a problem hiding this comment.
Could you add descriptions for these tests?
There was a problem hiding this comment.
Added missing descriptions for this test and an old test that I used as a template for this. I also converted both to use t.Run.
| @@ -952,6 +959,16 @@ func (m *Metrics) RecordRequestPrivacy(privacy metrics.PrivacyLabels) { | |||
| } | |||
| } | |||
|
|
|||
| func (m *Metrics) RecordAdapterBuyerUIDScrubbed(adapterName openrtb_ext.BidderName) { | |||
| if m.metricsDisabled.AdapterGDPRRequestBlocked { | |||
| return | |||
There was a problem hiding this comment.
Is it possible to add a test to cover this return?
There was a problem hiding this comment.
Added coverage and fixed the incorrect condition here.
exchange/utils.go
Outdated
| buyerUIDSet := false | ||
| if reqWrapper.User != nil && reqWrapper.User.BuyerUID != "" { | ||
| buyerUIDSet = true | ||
| } |
There was a problem hiding this comment.
buyerUIDSet := reqWrapper.User != nil && reqWrapper.User.BuyerUID != ""?
| @@ -202,6 +205,9 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context, | |||
| privacy.ScrubDeviceIDsIPsUserDemoExt(reqWrapper, ipConf, "eids", false) | |||
| } | |||
| } | |||
| if buyerUIDSet && reqWrapper.User.BuyerUID == "" { | |||
| rs.me.RecordAdapterBuyerUIDScrubbed(bidderRequest.BidderCoreName) | |||
| } | |||
There was a problem hiding this comment.
This works. Alternatively, we could track a second bool buyerIDRemoved which is set true alongside calls to any of the above privacy.Scrub calls.
metrics/go_metrics.go
Outdated
| @@ -926,6 +931,21 @@ func (me *Metrics) RecordRequestPrivacy(privacy PrivacyLabels) { | |||
| } | |||
| } | |||
|
|
|||
| func (me *Metrics) RecordAdapterBuyerUIDScrubbed(adapterName openrtb_ext.BidderName) { | |||
| adapterStr := string(adapterName) | |||
There was a problem hiding this comment.
Prefer adapterName.String()
There was a problem hiding this comment.
In order for config values be read from environment variables, Viper needs a v.SetDefault entry (I don't recall why). Can we add a SetDefault() call?
890 // Set the default config values for the viper object we are using.
891 func SetupViper(v *viper.Viper, filename string, bidderInfos BidderInfos) {
892 if filename != "" {
893 v.SetConfigName(filename)
894 *-- 56 lines: v.AddConfigPath(".")-------------------------------------------------------
950 v.SetDefault("metrics.disabled_metrics.adapter_connections_metrics", true)
951 v.SetDefault("metrics.disabled_metrics.adapter_gdpr_request_blocked", false)
+ v.SetDefault("metrics.disabled_metrics.adapter_buyeruid_scrubbed", false)
952 v.SetDefault("metrics.influxdb.host", "")
config/config.go
Also, for the sake of backwards consistency in terms of the extra resources PBS requires when Prometheus preloads one counter per adapter when adapter_buyeruid_scrubbed holds a value of false, should we default to true instead?
In my opinion, when users update their PBS instance, they shouldn't experience an extra resource demand unless they explicitly enable this new feature. Thoughts?
No description provided.