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.

Sign up for GitHub

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

Jump to bottom

Prometheus Refactor #1108

Merged
merged 12 commits into from
Nov 27, 2019
Merged

Prometheus Refactor #1108

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
565bb47
Initial Commit
SyntaxNode Nov 7, 2019
65227e2
Finished Design + Initial Tests
SyntaxNode Nov 8, 2019
f2b872a
Attempt #1 For Testify Equality Fix
SyntaxNode Nov 8, 2019
bdf4ad9
Attempt #2 For Testify Equality Fix
SyntaxNode Nov 8, 2019
3469dc2
Attempt #3 For Testify Equality Fix
SyntaxNode Nov 9, 2019
c3d8795
Expanded Tests
SyntaxNode Nov 11, 2019
0a95c52
Expanded Tests
SyntaxNode Nov 12, 2019
d8b9ba8
Finished tests. Fix misconfigured cardinality.
SyntaxNode Nov 19, 2019
d8cd3b8
PR Feedback
SyntaxNode Nov 21, 2019
43a44cb
PR Feedback + SetUserID Defensiveness
SyntaxNode Nov 25, 2019
b20a656
Clarified Prebid Cache Timing Metric
SyntaxNode Nov 25, 2019
5604600
GoLang Doesn't Like Underscores
SyntaxNode Nov 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion endpoints/cookie_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (deps *cookieSyncDeps) Endpoint(w http.ResponseWriter, r *http.Request, _ h

defer deps.pbsAnalytics.LogCookieSyncObject(&co)

deps.metrics.RecordCookieSync(pbsmetrics.Labels{})
deps.metrics.RecordCookieSync()
userSyncCookie := usersync.ParsePBSCookieFromRequest(r, deps.hostCookie)
if !userSyncCookie.AllowSyncs() {
http.Error(w, "User has opted out", http.StatusUnauthorized)
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ require (
github.com/onsi/ginkgo v1.10.1 // indirect
github.com/onsi/gomega v1.7.0 // indirect
github.com/pelletier/go-toml v1.2.0 // indirect
github.com/prebid/go-gdpr v0.6.0
github.com/prometheus/client_golang v0.0.0-20180623155954-77e8f2ddcfed
github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910
github.com/prometheus/common v0.0.0-20180801064454-c7de2306084e // indirect
Expand Down
3 changes: 3 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,11 @@ github.com/pelletier/go-toml v1.2.0 h1:T5zMGML61Wp+FlcbWjRDT7yAxhJNAiPPLOFECq181
github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/prebid/go-gdpr v0.6.0 h1:/GKrygGkUbsgd96HIkjAu7/6CHtRedvcojRtfAd4Igc=
github.com/prebid/go-gdpr v0.6.0/go.mod h1:FPY0uxSrl9/Mz237LnPo3ge4aCG0wQ9FWf2b4WhwNn0=
github.com/prometheus/client_golang v0.0.0-20180623155954-77e8f2ddcfed h1:0dloFFFNNDG7c+8qtkYw2FdADrWy9s5cI8wHp6tK3Mg=
github.com/prometheus/client_golang v0.0.0-20180623155954-77e8f2ddcfed/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXPKyh/dDVn+NZz0KFw=
github.com/prometheus/client_golang v1.2.1 h1:JnMpQc6ppsNgw9QPAGF6Dod479itz7lvlsMzzNayLOI=
github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910 h1:idejC8f05m9MGOsuEi1ATq9shN03HrxNkD/luQvxCv8=
github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo=
github.com/prometheus/common v0.0.0-20180801064454-c7de2306084e h1:n/3MEhJQjQxrOUCzh1Y3Re6aJUUWRp2M9+Oc3eVn/54=
Expand Down
13 changes: 6 additions & 7 deletions pbsmetrics/config/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ func (me *MultiMetricsEngine) RecordAdapterTime(labels pbsmetrics.AdapterLabels,
}

// RecordCookieSync across all engines
func (me *MultiMetricsEngine) RecordCookieSync(labels pbsmetrics.Labels) {
func (me *MultiMetricsEngine) RecordCookieSync() {
for _, thisME := range *me {
thisME.RecordCookieSync(labels)
thisME.RecordCookieSync()
}
}

Expand Down Expand Up @@ -175,9 +175,9 @@ func (me *MultiMetricsEngine) RecordUserIDSet(userLabels pbsmetrics.UserLabels)
}

// RecordPrebidCacheRequestTime across all engines
func (me *MultiMetricsEngine) RecordPrebidCacheRequestTime(labels pbsmetrics.RequestLabels, length time.Duration) {
func (me *MultiMetricsEngine) RecordPrebidCacheRequestTime(success bool, length time.Duration) {
for _, thisME := range *me {
thisME.RecordPrebidCacheRequestTime(labels, length)
thisME.RecordPrebidCacheRequestTime(success, length)
}
}

Expand Down Expand Up @@ -240,7 +240,7 @@ func (me *DummyMetricsEngine) RecordAdapterTime(labels pbsmetrics.AdapterLabels,
}

// RecordCookieSync as a noop
func (me *DummyMetricsEngine) RecordCookieSync(labels pbsmetrics.Labels) {
func (me *DummyMetricsEngine) RecordCookieSync() {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep consistency maybe we can remove this return statement as was done in the RecordPrebidCacheRequestTime function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

Expand All @@ -265,6 +265,5 @@ func (me *DummyMetricsEngine) RecordStoredImpCacheResult(cacheResult pbsmetrics.
}

// RecordPrebidCacheRequestTime as a noop
func (me *DummyMetricsEngine) RecordPrebidCacheRequestTime(labels pbsmetrics.RequestLabels, length time.Duration) {
return
func (me *DummyMetricsEngine) RecordPrebidCacheRequestTime(success bool, length time.Duration) {
}
5 changes: 1 addition & 4 deletions pbsmetrics/config/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,6 @@ func TestMultiMetricsEngine(t *testing.T) {
AudioImps: true,
NativeImps: true,
}
requestLabels := pbsmetrics.RequestLabels{
RequestStatus: pbsmetrics.RequestStatusOK,
}
for i := 0; i < 5; i++ {
metricsEngine.RecordRequest(labels)
metricsEngine.RecordImps(impTypeLabels)
Expand All @@ -88,7 +85,7 @@ func TestMultiMetricsEngine(t *testing.T) {
metricsEngine.RecordAdapterPrice(pubLabels, 1.34)
metricsEngine.RecordAdapterBidReceived(pubLabels, openrtb_ext.BidTypeBanner, true)
metricsEngine.RecordAdapterTime(pubLabels, time.Millisecond*20)
metricsEngine.RecordPrebidCacheRequestTime(requestLabels, time.Millisecond*20)
metricsEngine.RecordPrebidCacheRequestTime(true, time.Millisecond*20)
}
labelsBlacklist := []pbsmetrics.Labels{
{
Expand Down
11 changes: 5 additions & 6 deletions pbsmetrics/go_metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ func (me *Metrics) RecordAdapterTime(labels AdapterLabels, length time.Duration)
}

// RecordCookieSync implements a part of the MetricsEngine interface. Records a cookie sync request
func (me *Metrics) RecordCookieSync(labels Labels) {
func (me *Metrics) RecordCookieSync() {
me.CookieSyncMeter.Mark(1)
}

Expand Down Expand Up @@ -518,13 +518,12 @@ func (me *Metrics) RecordStoredImpCacheResult(cacheResult CacheResult, inc int)

// RecordPrebidCacheRequestTime implements a part of the MetricsEngine interface. Records the
// amount of time taken to store the auction result in Prebid Cache.
func (me *Metrics) RecordPrebidCacheRequestTime(labels RequestLabels, length time.Duration) {
if labels.RequestStatus == RequestStatusOK {
func (me *Metrics) RecordPrebidCacheRequestTime(success bool, length time.Duration) {
if success {
me.PrebidCacheRequestTimerSuccess.Update(length)
return
} else {
me.PrebidCacheRequestTimerError.Update(length)
}

me.PrebidCacheRequestTimerError.Update(length)
}

func doMark(bidder openrtb_ext.BidderName, meters map[openrtb_ext.BidderName]metrics.Meter) {
Expand Down
12 changes: 4 additions & 8 deletions pbsmetrics/go_metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,25 +172,21 @@ func TestNewMetricsWithDisabledConfig(t *testing.T) {
assert.True(t, m.MetricsDisabled.AccountAdapterDetails, "Accound adapter metrics should be disabled")
}

func TestRecordPrebidCacheRequestTimeWithSuccessLabel(t *testing.T) {
func TestRecordPrebidCacheRequestTimeWithSuccess(t *testing.T) {
registry := metrics.NewRegistry()
m := NewMetrics(registry, []openrtb_ext.BidderName{openrtb_ext.BidderAppnexus}, config.DisabledMetrics{AccountAdapterDetails: true})

m.RecordPrebidCacheRequestTime(RequestLabels{
RequestStatus: RequestStatusOK,
}, 42)
m.RecordPrebidCacheRequestTime(true, 42)

assert.Equal(t, m.PrebidCacheRequestTimerSuccess.Count(), int64(1))
assert.Equal(t, m.PrebidCacheRequestTimerError.Count(), int64(0))
}

func TestRecordPrebidCacheRequestTimeWithErrorLabel(t *testing.T) {
func TestRecordPrebidCacheRequestTimeWithNotSuccess(t *testing.T) {
registry := metrics.NewRegistry()
m := NewMetrics(registry, []openrtb_ext.BidderName{openrtb_ext.BidderAppnexus}, config.DisabledMetrics{AccountAdapterDetails: true})

m.RecordPrebidCacheRequestTime(RequestLabels{
RequestStatus: RequestStatusErr,
}, 42)
m.RecordPrebidCacheRequestTime(false, 42)

assert.Equal(t, m.PrebidCacheRequestTimerSuccess.Count(), int64(0))
assert.Equal(t, m.PrebidCacheRequestTimerError.Count(), int64(1))
Expand Down
14 changes: 12 additions & 2 deletions pbsmetrics/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,16 @@ const (
RequestActionErr RequestAction = "err"
)

// RequestActions returns possible setuid action labels
func RequestActions() []RequestAction {
return []RequestAction{
RequestActionSet,
RequestActionOptOut,
RequestActionGDPR,
RequestActionErr,
}
}

// MetricsEngine is a generic interface to record PBS metrics into the desired backend
// The first three metrics function fire off once per incoming request, so total metrics
// will equal the total numer of incoming requests. The remaining 5 fire off per outgoing
Expand All @@ -256,10 +266,10 @@ type MetricsEngine interface {
RecordAdapterBidReceived(labels AdapterLabels, bidType openrtb_ext.BidType, hasAdm bool)
RecordAdapterPrice(labels AdapterLabels, cpm float64)
RecordAdapterTime(labels AdapterLabels, length time.Duration)
RecordCookieSync(labels Labels) // May ignore all labels
RecordCookieSync()
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 labels are unused. Slimming down the interface.

RecordAdapterCookieSync(adapter openrtb_ext.BidderName, gdprBlocked bool)
RecordUserIDSet(userLabels UserLabels) // Function should verify bidder values
RecordStoredReqCacheResult(cacheResult CacheResult, inc int)
RecordStoredImpCacheResult(cacheResult CacheResult, inc int)
RecordPrebidCacheRequestTime(labels RequestLabels, length time.Duration)
RecordPrebidCacheRequestTime(success bool, length time.Duration)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really just care about success and failure. Changing to match similar existing constructs in the metrics.

}
8 changes: 4 additions & 4 deletions pbsmetrics/metrics_mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ func (me *MetricsEngineMock) RecordAdapterTime(labels AdapterLabels, length time
}

// RecordCookieSync mock
func (me *MetricsEngineMock) RecordCookieSync(labels Labels) {
me.Called(labels)
func (me *MetricsEngineMock) RecordCookieSync() {
me.Called()
}

// RecordAdapterCookieSync mock
Expand All @@ -93,6 +93,6 @@ func (me *MetricsEngineMock) RecordStoredImpCacheResult(cacheResult CacheResult,
}

// RecordPrebidCacheRequestTime mock
func (me *MetricsEngineMock) RecordPrebidCacheRequestTime(labels RequestLabels, length time.Duration) {
me.Called(labels, length)
func (me *MetricsEngineMock) RecordPrebidCacheRequestTime(success bool, length time.Duration) {
me.Called(success, length)
}
Loading