Add Native-Specific Metrics to Prebid Server #930

guscarreon · 2019-06-07T19:42:47Z

Prebid Server should be able to support in-app native requests to demand partners. The direct task is to track native both in Influx and Prometheus and doesn’t make sense to track native without tracking the other types

… right track here

pbsmetrics/metrics.go

josephveach

I don't know whether the document linked in the description provides adequate direction for what the native-specific metrics should be. It is clear there should be at least one adapter-specific error counter (see AdapterErrors() in metrics.go). I would guess that a counter of something at the request level, possibly number of native imps in the request, would also be desired. Should there be a meeting to flesh out the requirements?

BTW, the linked document in the description might not be visible outside Appnexus, in which case the link should probably be removed.

hhhjort

Where do we ever call these metrics? What about Prometheus support?

This is a partial setup to record per-Imp metrics, but no where in the code do we ever call the metrics engine on a per-imp basis. And no where in this code do we call the new counters that are being created.

exchange/exchange.go

hhhjort · 2019-06-14T19:26:39Z

pbsmetrics/go_metrics.go

@@ -317,6 +317,10 @@ func (me *Metrics) RecordRequest(labels Labels) {

 func (me *Metrics) RecordImps(labels Labels, numImps int) {
 	me.ImpMeter.Mark(int64(numImps))
+	me.ImpsTypeBanner.Mark(int64(labels.BannerImps))


Might be more efficient to put in if >0 on the counters before calling Mark() I am not sure that Mark() is smart enough to check for a no-op before engaging in all the thread safe concurrency machinery.

ok, working on it

Where is this done, and where is RecordImps() called from?

I'm sorry Hans, those changes I had not commited. The latest version has been pushed. deps.metricsEngine.RecordImps(labels, numImps) gets called from every endpoint's auction function
endpoints/openrtb2/auction.go

75 func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { 76 77 +-- 10 lines: ao := analytics.AuctionObject{----------------------------------------------------- 87 // to compute the auction timeout. 88 start := time.Now() 89 labels := pbsmetrics.Labels{ 90 Source: pbsmetrics.DemandUnknown, 91 RType: pbsmetrics.ReqTypeORTB2Web, 92 BannerImps: 0, 93 VideoImps: 0, 94 AudioImps: 0, 95 NativeImps: 0, 96 PubID: "", 97 Browser: pbsmetrics.BrowserOther, 98 CookieFlag: pbsmetrics.CookieFlagUnknown, 99 RequestStatus: pbsmetrics.RequestStatusOK, 100 } 101 numImps := 0 102 defer func() { 103 deps.metricsEngine.RecordRequest(labels) 104 deps.metricsEngine.RecordImps(labels, numImps) //<-- 105 deps.metricsEngine.RecordRequestTime(labels, time.Since(start)) 106 deps.analytics.LogAuctionObject(&ao) 107 }() 108 endpoints/openrtb2/auction.go

endpoints/openrtb2/amp_auction.go

74 func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { 75 76 +-- 14 lines: ao := analytics.AmpObject{---------------------------------------------------------- 90 start := time.Now() 91 labels := pbsmetrics.Labels{ 92 Source: pbsmetrics.DemandUnknown, 93 RType: pbsmetrics.ReqTypeAMP, 94 BannerImps: 0, 95 VideoImps: 0, 96 AudioImps: 0, 97 NativeImps: 0, 98 PubID: "", 99 Browser: pbsmetrics.BrowserOther, 100 CookieFlag: pbsmetrics.CookieFlagUnknown, 101 RequestStatus: pbsmetrics.RequestStatusOK, 102 } 103 defer func() { 104 deps.metricsEngine.RecordRequest(labels) 105 deps.metricsEngine.RecordImps(labels, 1) //<-- 106 deps.metricsEngine.RecordRequestTime(labels, time.Since(start)) 107 deps.analytics.LogAmpObject(&ao) 108 }() endpoints/openrtb2/amp_auction.go

endpoints/openrtb2/video_auction.go

66 func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { 67 68 ao := analytics.AuctionObject{ 69 Status: http.StatusOK, 70 Errors: make([]error, 0), 71 } 72 73 start := time.Now() 74 labels := pbsmetrics.Labels{ 75 Source: pbsmetrics.DemandUnknown, 76 RType: pbsmetrics.ReqTypeVideo, 77 BannerImps: 0, 78 VideoImps: 0, 79 AudioImps: 0, 80 NativeImps: 0, 81 PubID: "", 82 Browser: pbsmetrics.BrowserOther, 83 CookieFlag: pbsmetrics.CookieFlagUnknown, 84 RequestStatus: pbsmetrics.RequestStatusOK, 85 } 86 numImps := 0 87 defer func() { 88 deps.metricsEngine.RecordRequest(labels) 89 deps.metricsEngine.RecordImps(labels, numImps) //<-- 90 deps.metricsEngine.RecordRequestTime(labels, time.Since(start)) 91 deps.analytics.LogAuctionObject(&ao) 92 }() endpoints/openrtb2/video_auction.go

guscarreon · 2019-06-14T19:27:54Z

Where do we ever call these metrics? What about Prometheus support?

This is a partial setup to record per-Imp metrics, but no where in the code do we ever call the metrics engine on a per-imp basis. And no where in this code do we call the new counters that are being created.

Just pushed an update of this PR where I modify one of the metrics interface implemented functions. Prometheous support is still not ready though

hhhjort · 2019-06-14T19:42:06Z

Are you intending to add Prometheus support this PR, or should we be expecting a separate PR for that?

guscarreon · 2019-06-16T22:50:07Z

Are you intending to add Prometheus support this PR, or should we be expecting a separate PR for that?

I believe we can add it in this PR. Unless you feel it is better to handle it separately

hhhjort · 2019-06-25T14:58:50Z

pbsmetrics/prometheus/prometheus.go

@@ -16,6 +17,7 @@ type Metrics struct {
 	connCounter          prometheus.Gauge
 	connError            *prometheus.CounterVec
 	imps                 *prometheus.CounterVec
+	impTypes             *prometheus.CounterVec


I think it may be best to refactor RecordImps() rather than add impTypes as a redundant metric. The key here is prometheus is set up so it could easily track multiformat counts. The way it is currently coded, if you have an imp that allows both banner and video ads, and you ask what the total number of imps are, it will tell you two. But if you refactor how imps are recorded, you can preserve the information that there was one imp, and it was valid as both a banner imp and a video imp.

What we would want to do is loop over the imps, and call RecordImps() once per imp. We would make the values for the imp types boolean, and then add one imp recording with the proper flags for each call. This will allow us to track the true total number of imps along with counts for each imp type.

We probably want to have a new ImpLabels type to help keep things clean, and make it clear that the imp type booleans only apply to RecordImps() and not the other metrics.

This will also require the influx metrics to be refactored a bit.

hhhjort · 2019-07-01T17:36:22Z

pbsmetrics/prometheus/prometheus.go

@@ -45,7 +45,6 @@ func NewMetrics(cfg config.PrometheusMetrics) *Metrics {
 	adapterLabelNames := []string{"demand_source", "request_type", "browser", "cookie", "adapter_bid", "adapter"}
 	bidLabelNames := []string{"demand_source", "request_type", "browser", "cookie", "adapter_bid", "adapter", "bidtype", "markup_type"}
 	errorLabelNames := []string{"demand_source", "request_type", "browser", "cookie", "adapter_error", "adapter"}
-
 	impLabelNames := []string{"banner_imps", "video_imps", "audio_imps", "native_imps"}


Comparing to the latest master, I see all these label names have been converted to constants ... probably should do the same for impLabelNames

Also can drop _imps from the label name, as these are labels on the imps_requested_total counter. Looking at imps_requested_total.banner_imps looks a bit redundant. :)

hhhjort · 2019-07-01T17:38:55Z

pbsmetrics/prometheus/prometheus.go

-	metrics.Registry.MustRegister(metrics.imps)
-	metrics.impTypes = newCounter(cfg, "imps_types_total",
-		"Total number of impression types requested through PBS.",
+	metrics.imps = newCounter(cfg, "imps_requested_total_by_type",


Can shorten the name to imps_requested_total or even imps_requested ... the "by type" is implied by the labels available.

hhhjort

LGTM

mansinahar · 2019-07-15T18:27:48Z

@guscarreon Can you please resolve the conflicts on this one when you have a chance?

guscarreon · 2019-07-15T18:36:48Z

@guscarreon Can you please resolve the conflicts on this one when you have a chance?

Done Mansi, conlicts resolved

…into PBS-108 resolving travis conflicts

hhhjort

I am good with this addition. ;)

mansinahar

LGTM other than the 2 minor comments

pbsmetrics/config/metrics.go

pbsmetrics/prometheus/prometheus.go

mansinahar

LGTM other than the 2 minor comments

* going out real quick, brb * brb to attend 395 * Test cases still not ready, but touching base to see if we are on the right track here * removed some commented lines * Counting Imp types inside go_metrics.RecordImps function * Added >0 check in go_metrics.RecordImps * working on the prometheus part * going out real quick * Finally working as a counterVector for banner. Replicate for the others * Prometheus implemented * Test case for RecordImps() function * refactored imp types to be boolean values inside a new ImpLabels type * refactoring prometheus.go * Hans' refactoring and RecordLegacyImps() implemented * corrected .travis.yml * trying to fix travis issue * Small changes after Mansi's review

Gus Carreon added 3 commits June 5, 2019 14:14

going out real quick, brb

00f48e3

brb to attend 395

6d8bf61

Test cases still not ready, but touching base to see if we are on the…

654fb2c

… right track here

guscarreon requested review from mkendall07, mansinahar, hhhjort and josephveach June 7, 2019 19:42

hhhjort reviewed Jun 7, 2019

View reviewed changes

pbsmetrics/metrics.go Outdated Show resolved Hide resolved

removed some commented lines

aaa9a00

josephveach reviewed Jun 11, 2019

View reviewed changes

guscarreon assigned hhhjort Jun 14, 2019

hhhjort requested changes Jun 14, 2019

View reviewed changes

Counting Imp types inside go_metrics.RecordImps function

5d7fa8f

hhhjort reviewed Jun 14, 2019

View reviewed changes

exchange/exchange.go Show resolved Hide resolved

hhhjort reviewed Jun 14, 2019

View reviewed changes

Added >0 check in go_metrics.RecordImps

bcd4b12

Gus Carreon added 5 commits June 17, 2019 15:59

working on the prometheus part

d484938

going out real quick

d5a6afe

Finally working as a counterVector for banner. Replicate for the others

d919002

Prometheus implemented

5d76416

Test case for RecordImps() function

7661993

hhhjort reviewed Jun 25, 2019

View reviewed changes

refactored imp types to be boolean values inside a new ImpLabels type

73e7676

hhhjort reviewed Jul 1, 2019

View reviewed changes

Gus Carreon added 2 commits July 9, 2019 11:27

refactoring prometheus.go

569e649

Hans' refactoring and RecordLegacyImps() implemented

44096ba

hhhjort previously approved these changes Jul 11, 2019

View reviewed changes

hhhjort assigned josephveach Jul 11, 2019

guscarreon assigned mansinahar and unassigned hhhjort and josephveach Jul 15, 2019

Merge branch 'master' into PBS-108

63dd6c1

guscarreon dismissed hhhjort’s stale review via 63dd6c1 July 15, 2019 18:36

Gus Carreon added 3 commits July 16, 2019 11:09

corrected .travis.yml

5c6442c

Merge branch 'PBS-108' of https://github.com/guscarreon/prebid-server …

789d60f

…into PBS-108 resolving travis conflicts

trying to fix travis issue

36e4a6a

hhhjort previously approved these changes Jul 16, 2019

View reviewed changes

mansinahar previously approved these changes Jul 17, 2019

View reviewed changes

pbsmetrics/config/metrics.go Outdated Show resolved Hide resolved

pbsmetrics/prometheus/prometheus.go Outdated Show resolved Hide resolved

Small changes after Mansi's review

f0d2976

guscarreon dismissed stale reviews from mansinahar and hhhjort via f0d2976 July 17, 2019 19:08

mansinahar approved these changes Jul 18, 2019

View reviewed changes

guscarreon merged commit 92d91ae into prebid:master Jul 18, 2019

guscarreon deleted the PBS-108 branch July 18, 2019 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Native-Specific Metrics to Prebid Server #930

Add Native-Specific Metrics to Prebid Server #930

guscarreon commented Jun 7, 2019 •

edited

Loading

josephveach left a comment

hhhjort left a comment

hhhjort Jun 14, 2019

guscarreon Jun 14, 2019

guscarreon Jun 17, 2019

hhhjort Jun 17, 2019

guscarreon Jun 17, 2019

guscarreon commented Jun 14, 2019

hhhjort commented Jun 14, 2019

guscarreon commented Jun 16, 2019

hhhjort Jun 25, 2019

hhhjort Jul 1, 2019

hhhjort Jul 1, 2019

hhhjort Jul 1, 2019

hhhjort left a comment

mansinahar commented Jul 15, 2019

guscarreon commented Jul 15, 2019

hhhjort left a comment

mansinahar left a comment

mansinahar left a comment

Add Native-Specific Metrics to Prebid Server #930

Add Native-Specific Metrics to Prebid Server #930

Conversation

guscarreon commented Jun 7, 2019 • edited Loading

josephveach left a comment

Choose a reason for hiding this comment

hhhjort left a 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

guscarreon commented Jun 14, 2019

hhhjort commented Jun 14, 2019

guscarreon commented Jun 16, 2019

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

mansinahar commented Jul 15, 2019

guscarreon commented Jul 15, 2019

hhhjort left a comment

Choose a reason for hiding this comment

mansinahar left a comment

Choose a reason for hiding this comment

mansinahar left a comment

Choose a reason for hiding this comment

guscarreon commented Jun 7, 2019 •

edited

Loading