Add Outgoing Connection Metrics #1343

guscarreon · 2020-06-08T06:20:56Z

Add metrics to track the number of connections per host and differentiate between an actively used connection vs an idle connection.

Technical Reference:
https://medium.com/@deeeet/trancing-http-request-latency-in-golang-65b2463f548c
https://blog.golang.org/http-tracing

SyntaxNode · 2020-06-08T15:14:35Z

pbsmetrics/go_metrics.go

@@ -70,6 +70,11 @@ type AdapterMetrics struct {
 	BidsReceivedMeter metrics.Meter
 	PanicMeter        metrics.Meter
 	MarkupMetrics     map[openrtb_ext.BidType]*MarkupDeliveryMetrics
+	ConnSuccess       metrics.Counter


As you suspect, we may not need all of this information. Here's my take:

ConnSuccess is the default assumption. I don't think we need to log that counter. This also looks like the sum of ConnCreated and ConnReused which can be computed easily in queries if desired.

ConnError is likely already surfaced with other metrics. I don't remember exactly. I think it either turns into a panic or an adapter error. Thoughts on if this is worth a counter?

ConnCreated seems very useful in tracking connection management.

ConnReused seems very useful in tracking connection management.

There's also two metrics I'd like to throw out for consideration:

Connection wait time. This one is a bit tricky and might not easily be done with the built-in tracers. The idea is to measure the time when the connection request is first made until a connection is either created or reused. This will tell us how bad the congestion in the connection pool is and how much delay is being introduced. This would be expensive, for sure... likely 12 additional metrics (at least for Prometheus) per adapter... which is now roughly 840 in total. Still, I think it would be super useful information.

Dns resolution wait time. I don't think this is worthwhile to record per adapter, but might be interesting in aggregate for hosts to understand how their DNS resolution layer is performing.

I agree with @SyntaxNode. We only need to know if a new connection was created for a bidder vs if an existing connection was reused. Either of those would mean that we were able to connect with the bidder successfully. Note that we already have a metric called, adapter_errors which should record any type of adapter related error including not being able to connect to the adapter.

I think adding connection wait time metric is worth it because we wanna make sure that our suspicion that creating new connections is the cause of increased latencies during spikey traffic, is indeed true. I believe you should be able to do that by recording the timestamp in GetConn() and GotConn() and then subtracting the two. This will represent the entire time taken to create a new connection (including DNS lookup and TLS handshake time). Although, when we record this, we wanna make sure that we're recording it only when we're creating a new connection and not when we are reusing an existing one.

One other thing to consider is redirects. In case of redirects, these trace functions may be called multiple times which will then in turn record these metrics multiple time for each redirect connection as well. This could lead to misleading numbers in our metrics. This one is a little tricky and will require a little more thought as to how we can handle this.

SyntaxNode · 2020-06-08T15:24:09Z

pbsmetrics/prometheus/prometheus.go

@@ -178,6 +185,11 @@ func NewMetrics(cfg config.PrometheusMetrics) *Metrics {
 		"Count of requests labeled by adapter, if has a cookie, and if it resulted in bids.",
 		[]string{adapterLabel, cookieLabel, hasBidsLabel})

+	metrics.adapterConnections = newCounter(cfg, metrics.Registry,
+		"adapter_connections",
+		"Count of connections to bidder hosts labeled by their adapters. Tracks if connection was successful, connection was reused, or if connection was left idle",


These should be separated out into their own metrics. I don't think the correlation serves meaningful purposes here. Some of the labels are mutually exclusive (success vs error, reused vs created) and knowing the relation between reused vs created and idle doesn't seem very interesting.

SyntaxNode · 2020-06-08T15:25:09Z

pbsmetrics/prometheus/prometheus_test.go

@@ -61,7 +61,7 @@ func TestMetricCountGatekeeping(t *testing.T) {
 	// Verify Per-Adapter Cardinality
 	// - This assertion provides a warning for newly added adapter metrics. Threre are 40+ adapters which makes the
 	//   cost of new per-adapter metrics rather expensive. Thought should be given when adding new per-adapter metrics.
-	assert.True(t, perAdapterCardinalityCount <= 22, "Per-Adapter Cardinality")
+	assert.True(t, perAdapterCardinalityCount <= 54, "Per-Adapter Cardinality")


Yeah, see? With having these as labels on the same adapter the cardinality is very high for the information we're recording and we're adding 32 new metrics instead of just 5 per adapter.

Update with version 0.115.0 changes

Update with latest changes

guscarreon · 2020-06-20T01:31:53Z

Below is the diff command output in prometheus metrics after a post request to bidder appnexus

A new connection and connection wait time

168c168
< prebid_server_adapter_connection_created{adapter="appnexus"} 0
---
> prebid_server_adapter_connection_created{adapter="appnexus"} 1
375a376,390
> # HELP prebid_server_adapter_connection_wait Seconds from when the connection was requested until it is either created or reused
> # TYPE prebid_server_adapter_connection_wait histogram
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.05"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.1"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.15"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.2"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.25"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.3"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.4"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.5"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.75"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="1"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="+Inf"} 1
> prebid_server_adapter_connection_wait_sum{adapter="appnexus"} 0.03102841
> prebid_server_adapter_connection_wait_count{adapter="appnexus"} 1
602c617

DNS lookup metrics

< prebid_server_connections_closed 0
---
> prebid_server_connections_closed 1
4562c4577
< prebid_server_connections_opened 0
---
> prebid_server_connections_opened 1
4568,4580c4583,4595
< prebid_server_dns_lookup_time_bucket{le="0.05"} 0
< prebid_server_dns_lookup_time_bucket{le="0.1"} 0
< prebid_server_dns_lookup_time_bucket{le="0.15"} 0
< prebid_server_dns_lookup_time_bucket{le="0.2"} 0
< prebid_server_dns_lookup_time_bucket{le="0.25"} 0
< prebid_server_dns_lookup_time_bucket{le="0.3"} 0
< prebid_server_dns_lookup_time_bucket{le="0.4"} 0
< prebid_server_dns_lookup_time_bucket{le="0.5"} 0
< prebid_server_dns_lookup_time_bucket{le="0.75"} 0
< prebid_server_dns_lookup_time_bucket{le="1"} 0
< prebid_server_dns_lookup_time_bucket{le="+Inf"} 0
< prebid_server_dns_lookup_time_sum 0
< prebid_server_dns_lookup_time_count 0
---
> prebid_server_dns_lookup_time_bucket{le="0.05"} 1
> prebid_server_dns_lookup_time_bucket{le="0.1"} 1
> prebid_server_dns_lookup_time_bucket{le="0.15"} 1
> prebid_server_dns_lookup_time_bucket{le="0.2"} 1
> prebid_server_dns_lookup_time_bucket{le="0.25"} 1
> prebid_server_dns_lookup_time_bucket{le="0.3"} 1
> prebid_server_dns_lookup_time_bucket{le="0.4"} 1
> prebid_server_dns_lookup_time_bucket{le="0.5"} 1
> prebid_server_dns_lookup_time_bucket{le="0.75"} 1
> prebid_server_dns_lookup_time_bucket{le="1"} 1
> prebid_server_dns_lookup_time_bucket{le="+Inf"} 1
> prebid_server_dns_lookup_time_sum 0.022126272
> prebid_server_dns_lookup_time_count 1
4587c4602

mansinahar · 2020-06-22T16:01:31Z

pbsmetrics/go_metrics.go

+		ConnError:         metrics.NilCounter{},
+		ConnCreated:       metrics.NilCounter{},
+		ConnReused:        metrics.NilCounter{},
+		ConnIdleTime:      &metrics.NilTimer{},


Just curious, what would be the use case for ConnIdleTime metric?

I thought it'd be useful to know which connections are more efficient. I'm not sure if I'm going for a "nice to have" here instead of a "must have" or we "definitely need" this. Do think we should discard them?

mansinahar · 2020-06-22T16:04:06Z

pbsmetrics/go_metrics.go

@@ -234,6 +244,11 @@ func makeBlankAdapterMetrics() *AdapterMetrics {
 		BidsReceivedMeter: blankMeter,
 		PanicMeter:        blankMeter,
 		MarkupMetrics:     makeBlankBidMarkupMetrics(),
+		ConnError:         metrics.NilCounter{},


I think we can skip ConnError metric as we're only calling the RecordAdapterConnections from the GotConn method and the fact that the code got to that piece of the code means that we were able to successfully get a connection. Also, we have another metric called adapter_errors which I believe should record connection related errors for adapters as well. We might wanna check what is a connection error reported as in the adapter_error metric and perhaps add a new error type if needed.

Pretty good catch! The error metrics were not getting updated at all. I can remove them if you want, for the moment I left them there because I think it'd be interesting to know whether or not an adapter endpoint is having connection issues. But I do realize this is probably just a "nice to have" and not a "we definitely need this". We can discuss this in standup if you want and I can removed them if you want me to.

mansinahar · 2020-06-23T10:59:57Z

exchange/bidder.go

+// This function adds an httptrace.ClientTrace object to the context so, if connection with the bidder
+// endpoint is established, we can keep track of whether the connection was newly created, reused, and
+// even know if it was left idle and for how much time.
+func (bidder *bidderAdapter) AddClientTrace(ctx context.Context) context.Context {


What's the rationale behind having this method defined on the bidderAdapter receiver and also with keeping it exported?

Good point. Let me go back to the whiteboard. Maybe we can modify the context in exchange/exchange.go.

mansinahar · 2020-06-23T11:31:13Z

pbsmetrics/go_metrics_test.go


 	"github.com/prebid/prebid-server/config"
 	"github.com/prebid/prebid-server/openrtb_ext"
 	metrics "github.com/rcrowley/go-metrics"
 	"github.com/stretchr/testify/assert"
 )

+const OneSecond time.Duration = time.Second


time.Second is already a constant, any reason why we wanna define it as a constant again over here with a diff name?

Good point. Corrected

mansinahar · 2020-06-23T11:34:31Z

pbsmetrics/metrics_mock.go

+
+// RecordDNSTime mock
+func (me *MetricsEngineMock) RecordDNSTime(dnsLookupTime time.Duration) {
+}


Why don't we call the Called function for this method? Looks like something you might have missed by mistake?

Not necessary for TestRecordDNSTime

I still recommend to add the calls, so future tests can rely on that behavior if needed in the future.

mansinahar · 2020-06-23T11:39:28Z

pbsmetrics/prometheus/prometheus.go

@@ -63,6 +70,10 @@ const (
 	requestStatusLabel   = "request_status"
 	requestTypeLabel     = "request_type"
 	successLabel         = "success"
+
+	adapterConnErrorLabel   = "adapter_connection_error"


Since you've defined separate metrics for ConnError, ConnReused and ConnCreated, any reason why we also need labels? Also, they don't seem to be used anywhere.

Pretty good catch. This is a leftover from the previous commit. Removed

SyntaxNode · 2020-06-23T14:43:21Z

exchange/bidder.go

@@ -321,6 +324,7 @@ func (bidder *bidderAdapter) doRequest(ctx context.Context, req *adapters.Reques
 	}
 	httpReq.Header = req.Headers

+	ctx = bidder.AddClientTrace(ctx)


Should we add a feature flag around this option? Just in case it doesn't work as expected for some reason in prod or if a host would like to disable the feature to save a little bit in memory usage. If so, the prometheus preload should also only preload the metrics if the feature flag is enabled.

Added. Let me know your thoughts with the addition

guscarreon · 2020-06-26T02:37:23Z

Modified according to feedback. Kept both the error connection metrics and the connection idle time. Added a configuration boolean flag to disable the adapter connection metrics.

Ran the server locally and posted a request to check out if the metrics work as expected. These are the metrics before and after said request to the appnexus bidder.

170c170
< prebid_server_adapter_connection_created{adapter="appnexus"} 0
---
> prebid_server_adapter_connection_created{adapter="appnexus"} 1
578,590c578,590
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.05"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.1"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.15"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.2"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.25"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.3"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.4"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.5"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.75"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="1"} 0
< prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="+Inf"} 0
< prebid_server_adapter_connection_wait_sum{adapter="appnexus"} 0
< prebid_server_adapter_connection_wait_count{adapter="appnexus"} 0
---
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.05"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.1"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.15"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.2"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.25"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.3"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.4"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.5"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="0.75"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="1"} 1
> prebid_server_adapter_connection_wait_bucket{adapter="appnexus",le="+Inf"} 1
> prebid_server_adapter_connection_wait_sum{adapter="appnexus"} 0.036550788
> prebid_server_adapter_connection_wait_count{adapter="appnexus"} 1
    .
    .
    .
5581c5581
< prebid_server_connections_closed 0
---
> prebid_server_connections_closed 1
5588c5588
< prebid_server_connections_opened 0
---
> prebid_server_connections_opened 1
5594,5606c5594,5606
< prebid_server_dns_lookup_time_bucket{le="0.05"} 0
< prebid_server_dns_lookup_time_bucket{le="0.1"} 0
< prebid_server_dns_lookup_time_bucket{le="0.15"} 0
< prebid_server_dns_lookup_time_bucket{le="0.2"} 0
< prebid_server_dns_lookup_time_bucket{le="0.25"} 0
< prebid_server_dns_lookup_time_bucket{le="0.3"} 0
< prebid_server_dns_lookup_time_bucket{le="0.4"} 0
< prebid_server_dns_lookup_time_bucket{le="0.5"} 0
< prebid_server_dns_lookup_time_bucket{le="0.75"} 0
< prebid_server_dns_lookup_time_bucket{le="1"} 0
< prebid_server_dns_lookup_time_bucket{le="+Inf"} 0
< prebid_server_dns_lookup_time_sum 0
< prebid_server_dns_lookup_time_count 0
---
> prebid_server_dns_lookup_time_bucket{le="0.05"} 1
> prebid_server_dns_lookup_time_bucket{le="0.1"} 1
> prebid_server_dns_lookup_time_bucket{le="0.15"} 1
> prebid_server_dns_lookup_time_bucket{le="0.2"} 1
> prebid_server_dns_lookup_time_bucket{le="0.25"} 1
> prebid_server_dns_lookup_time_bucket{le="0.3"} 1
> prebid_server_dns_lookup_time_bucket{le="0.4"} 1
> prebid_server_dns_lookup_time_bucket{le="0.5"} 1
> prebid_server_dns_lookup_time_bucket{le="0.75"} 1
> prebid_server_dns_lookup_time_bucket{le="1"} 1
> prebid_server_dns_lookup_time_bucket{le="+Inf"} 1
> prebid_server_dns_lookup_time_sum 0.022831942
> prebid_server_dns_lookup_time_count 1

SyntaxNode · 2020-06-30T20:01:45Z

config/config.go

@@ -675,6 +680,7 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("http_client_cache.idle_connection_timeout_seconds", 60)
 	// no metrics configured by default (metrics{host|database|username|password})
 	v.SetDefault("metrics.disabled_metrics.account_adapter_details", false)
+	v.SetDefault("metrics.disabled_metrics.adapter_connections_metrics", false)


Open Question: Should make this disabled by default?

Makes sense, set to true so they are disabled by default.

SyntaxNode · 2020-06-30T21:32:31Z

exchange/bidder.go

+
+type bidderMetrics struct {
+	engine   pbsmetrics.MetricsEngine
+	disabled config.DisabledMetrics


I would normally expect a property called disabled to be a bool. disabledMetrics perhaps?

Makes total sense. Corrected.

SyntaxNode · 2020-06-30T21:40:30Z

exchange/bidder.go

@@ -334,6 +347,7 @@ func (bidder *bidderAdapter) doRequest(ctx context.Context, req *adapters.Reques
 			}

 		}
+		bidder.me.engine.RecordAdapterFailedConnections(bidder.BidderName)


I'm curious about this. A connection may fail due to our context deadline, which I don't think should be reported as a connection error. Are there other ways for ctxhttp.Do to error which has nothing to do with retrieving a connection?

True. This metric will be removed per our conversation

SyntaxNode · 2020-06-30T21:44:26Z

pbsmetrics/go_metrics.go

+	am.ConnError = metrics.GetOrRegisterCounter(fmt.Sprintf("%[1]s.%[2]s.connections_error", adapterOrAccount, exchange), registry)
+	am.ConnCreated = metrics.GetOrRegisterCounter(fmt.Sprintf("%[1]s.%[2]s.connections_created", adapterOrAccount, exchange), registry)
+	am.ConnReused = metrics.GetOrRegisterCounter(fmt.Sprintf("%[1]s.%[2]s.connections_reused", adapterOrAccount, exchange), registry)
+	am.ConnIdleTime = metrics.GetOrRegisterTimer(fmt.Sprintf("%[1]s.%[2]s.connection_idle_time", adapterOrAccount, exchange), registry)


I think this was asked before, but I forgot the answer. Per-adapter metrics are very expensive, especially for timers. Adding a timer for Prometheus results in nearly 1,000 new unique metrics. We'll use ConnWaitTime to tweak connection pools and so I think it earns it worth. What will we use ConnIdleTime for?

Per our conversation, ConnIdleTime was removed

SyntaxNode · 2020-06-30T21:46:36Z

pbsmetrics/metrics_mock.go

+
+// RecordDNSTime mock
+func (me *MetricsEngineMock) RecordDNSTime(dnsLookupTime time.Duration) {
+}


I still recommend to add the calls, so future tests can rely on that behavior if needed in the future.

mansinahar · 2020-07-16T12:06:26Z

exchange/bidder.go

+
+type bidderMetrics struct {
+	engine          pbsmetrics.MetricsEngine
+	disabledMetrics config.DisabledMetrics
 }

 func (bidder *bidderAdapter) requestBid(ctx context.Context, request *openrtb.BidRequest, name openrtb_ext.BidderName, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo) (*pbsOrtbSeatBid, []error) {


Since you've added BidderName to the bidderAdapter struct, can we please remove the name parameter from this function args list as it is redundant?

exchange/legacy.go's implementation of requestBid does use this parameter. Do you think it is better to keep it? I know it's unnecessary in every implementation of requestBid out there except for exchange/legacy.go, but for the moment, it is better to keep it to not break anything.

Another option is to eliminate bidderAdapter's BidderName field and move ctx = bidder.addClientTrace(ctx) upwards from doRequest to requestBid(ctx context.Context, request *openrtb.BidRequest, bidAdjustment float64, conversions currencies.Conversions, reqInfo *adapters.ExtraRequestInfo) in order to make use of its name argument. Though I bet it is better not to rely on name given that in the future legacy.go will eventually be dropped and we want less arguments in function signatures. What do you think?

Ah ok! Yeah, requestBid is part of the interface so I guess we'll have to let it be for now.

mansinahar · 2020-07-20T17:23:21Z

config/config.go

+
+	// True if we don't want to collect metrics about the connections prebid
+	// server establishes with bidder servers such as the number of connections
+	// that were created or reused and the connection idle and waiting times.


You might wanna remove the connection idle time from this comment as well as we're no longer recording that.

mansinahar · 2020-07-20T17:30:52Z

exchange/bidder.go

+		Client:     client,
+		me:         me,
+		config: bidderAdapterConfig{
+			DebugConfig:          cfg.Debug,


Please consider naming this as just Debug as the struct name itself has config in it.

mansinahar · 2020-07-20T17:42:09Z

exchange/bidder.go

+		me:         me,
+		config: bidderAdapterConfig{
+			DebugConfig:          cfg.Debug,
+			LogConnectionMetrics: cfg.Metrics.Disabled.AdapterConnectionMetrics,


Please consider naming this DisableConnMetrics or something along those lines because if this value is true, we won't record the connection metrics but the name LogConnectionMetrics indicates that we will.

mansinahar · 2020-07-21T02:53:50Z

pbsmetrics/config/metrics.go

+func (me *DummyMetricsEngine) RecordAdapterConnections(bidderName openrtb_ext.BidderName, info httptrace.GotConnInfo, obtainConnectionTime time.Duration) {
+}
+
+// Times the DNS resolution process


Nit: Please consider changing this comment to be RecordDNSTime as a noop for consistency purpose

mansinahar · 2020-07-21T02:57:33Z

pbsmetrics/go_metrics.go

@@ -268,6 +275,11 @@ func makeBlankAdapterMetrics() *AdapterMetrics {
 		PanicMeter:        blankMeter,
 		MarkupMetrics:     makeBlankBidMarkupMetrics(),
 	}
+	if disabledMetrics.AdapterConnectionMetrics {


Shouldn't this be if !disabledMetrics.AdapterConnectionMetrics? If disabledMetrics.AdapterConnectionMetrics is set to true then that means we don't want to record those metrics, right?

Totally right. That's a bug

mansinahar · 2020-07-21T03:52:56Z

exchange/bidder_test.go

+	// Setup mock server
+	respStatus := 200
+	respBody := "{\"bid\":false}"
+	server := httptest.NewServer(mockHandler(respStatus, "getBody", respBody))


Please consider passing the getBody param as an empty string here rather than the literal getBody. From what I understand you don't care about get body here and just post body. An empty strings represents that better IMHO

Many other tests do the exact same. I'm leaving it for consistency. A future PR can probably clean, refactor and make the code a bit easier to read in the entire file.

mansinahar · 2020-07-21T04:06:24Z

exchange/bidder_test.go

+			Body:    []byte("{\"key\":\"val\"}"),
+			Headers: http.Header{},
+		},
+		bidResponse: mockBidderResponse,


We can just have the bidResponse here to be an empty adapters.BidResponse struct as we don't really care about the bid response in this case. Or the other thing you can do is extract this single bid response to a global variable in this test file because I am sure this same response is used elsewhere in this test file too. This will reduce the redundant code.

You are right. Looks better. Done.

mansinahar · 2020-07-21T04:13:51Z

exchange/bidder_test.go

+	}
+
+	// Run test
+	bidder.doRequest(context.Background(), &adapters.RequestData{Method: "POST", Uri: "http://www.example.com/"})


Can't we have the above test testing RecordAdapterConnections also just go through doRequest rather than requestBid as you're introducing the change in the doRequestImpl function and so testing on that level for unit tests should be enough, no?

Does it hurt to go through that extra layer?

Well, for unit tests it is better if we keep the tests close to the functions that are being modified because say tomorrow if somebody makes some changes to the requestBid function that has nothing to do directly with the connection metrics but can potentially lead to the failure of this test.

mansinahar · 2020-07-21T04:19:30Z

pbsmetrics/go_metrics_test.go

+	}{
+		{
+			description: "Five second DNS lookup time",
+			in: testIn{


For this test, I guess we can skip wrapping the dnsLookupDuration and expDuration in testIn and testOut structs as they have only those single fields

Makes sense. Modified

mansinahar · 2020-07-21T04:20:31Z

pbsmetrics/go_metrics_test.go

+
+		m.RecordDNSTime(test.in.dnsLookupDuration)
+
+		assert.Equal(t, test.out.expDuration.Nanoseconds(), m.DNSLookupTimer.Sum(), "Test [%d] incorrect DNS lookup time", i)


Might be good to add the test description as part of the error message here rather than the test case index so that the reader doesn't necessarily have to come back here to map the index to the test case

The index was supposed to be for testing purposes. Modified.

mansinahar · 2020-07-22T16:22:38Z

exchange/bidder_test.go

+	}
+
+	// Run test
+	bidder.doRequest(context.Background(), &adapters.RequestData{Method: "POST", Uri: "http://www.example.com/"})


Well, for unit tests it is better if we keep the tests close to the functions that are being modified because say tomorrow if somebody makes some changes to the requestBid function that has nothing to do directly with the connection metrics but can potentially lead to the failure of this test.

first draft test cases still pending

9f596ca

guscarreon requested review from SyntaxNode and hhhjort June 8, 2020 06:23

guscarreon marked this pull request as draft June 8, 2020 06:41

Capitalization and counters

36e9a68

SyntaxNode reviewed Jun 8, 2020

View reviewed changes

Gus Carreon added 3 commits June 9, 2020 15:30

Merge branch 'master' into PBS-577

b49f5bf

Update with version 0.115.0 changes

corrections

7139a73

influxDB test case

4e10755

mansinahar self-assigned this Jun 16, 2020

Gus Carreon and others added 7 commits June 16, 2020 14:35

Merge branch 'master' into PBS-577

9eed763

Update with latest changes

checkout real quick

9d30a3f

corrected prometheus counter vectors

4631b87

Added TestRecordAdapterConnectionsCall

26829ce

Added connection wait time functionality and tests

4a905ce

Added DNS lookup metrics with tests

6498bd2

added preload call for adapter connection counter vector

0f27f69

guscarreon marked this pull request as ready for review June 20, 2020 01:26

Gus Carreon added 2 commits June 19, 2020 21:36

Corrected prometheus adapter metrics cardinality test

f3203cb

Merged latest pbs updates

0232fb7

guscarreon force-pushed the PBS-577 branch from 0aa7e84 to 0232fb7 Compare June 20, 2020 01:43

SyntaxNode self-assigned this Jun 22, 2020

mansinahar reviewed Jun 23, 2020

View reviewed changes

SyntaxNode reviewed Jun 23, 2020

View reviewed changes

Gus Carreon added 3 commits June 24, 2020 19:21

Corrected mechanism to record adapter connection errors

ec07dab

Got rid of unnecessary constants

f406940

added disable adapter connection metrics config flag

3fe8d52

SyntaxNode reviewed Jun 30, 2020

View reviewed changes

mansinahar reviewed Jul 16, 2020

View reviewed changes

Gus Carreon and others added 2 commits July 16, 2020 14:22

refactored the bidderAdapter struct

ca0e6be

Fix bidder.go and prometheus.go merge conflicts

d922b12

mansinahar reviewed Jul 21, 2020

View reviewed changes

Corrections after Mansi's review

acbae97

mansinahar approved these changes Jul 22, 2020

View reviewed changes

SyntaxNode approved these changes Jul 22, 2020

View reviewed changes

mansinahar merged commit a5962de into prebid:master Jul 22, 2020


		m.RecordDNSTime(test.in.dnsLookupDuration)

		assert.Equal(t, test.out.expDuration.Nanoseconds(), m.DNSLookupTimer.Sum(), "Test [%d] incorrect DNS lookup time", i)

Add Outgoing Connection Metrics #1343

Add Outgoing Connection Metrics #1343

Conversation

guscarreon commented Jun 8, 2020 • 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

guscarreon commented Jun 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar Jun 23, 2020 • 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

guscarreon commented Jun 26, 2020

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

mansinahar Jul 16, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar Jul 21, 2020 • 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

guscarreon commented Jun 8, 2020 •

edited

Loading

mansinahar Jun 22, 2020 •

edited

Loading

mansinahar Jun 23, 2020 •

edited

Loading

mansinahar Jul 16, 2020 •

edited

Loading

mansinahar Jul 21, 2020 •

edited

Loading