From d2883f12c1b43720f95827095cfcac21733e13de Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Date: Thu, 28 Sep 2017 14:25:22 -0700
Subject: [PATCH] Remove TimingDuration call from VA (#3122)

Also switch over tests.

Fixes #3100
---
 test/test-tools.go | 19 ++++++++++++++++---
 va/va.go           |  1 -
 va/va_test.go      | 46 +++++++++++++++++++++++++---------------------
 3 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/test/test-tools.go b/test/test-tools.go
index 3f8e80fe1..9b7218d1e 100644
--- a/test/test-tools.go
+++ b/test/test-tools.go
@@ -157,7 +157,7 @@ func AssertBetween(t *testing.T, a, b, c int64) {
 	}
 }
 
-// CountCounterVecVec returns the count by label and value of a prometheus metric
+// CountCounterVec returns the count by label and value of a prometheus metric
 func CountCounterVec(labelName string, value string, counterVec *prometheus.CounterVec) int {
 	return CountCounter(counterVec.With(prometheus.Labels{labelName: value}))
 }
@@ -169,11 +169,24 @@ func CountCounter(counter prometheus.Counter) int {
 	var m prometheus.Metric
 	select {
 	case <-time.After(time.Second):
-		fmt.Println("timed out collecting metrics")
-		return 0
+		panic("timed out collecting metrics")
 	case m = <-ch:
 	}
 	var iom io_prometheus_client.Metric
 	_ = m.Write(&iom)
 	return int(iom.Counter.GetValue())
 }
+
+func CountHistogramSamples(hist prometheus.Histogram) int {
+	ch := make(chan prometheus.Metric, 10)
+	hist.Collect(ch)
+	var m prometheus.Metric
+	select {
+	case <-time.After(time.Second):
+		panic("timed out collecting metrics")
+	case m = <-ch:
+	}
+	var iom io_prometheus_client.Metric
+	_ = m.Write(&iom)
+	return int(iom.Histogram.GetSampleCount())
+}
diff --git a/va/va.go b/va/va.go
index 24d2a5cfa..dfc173319 100644
--- a/va/va.go
+++ b/va/va.go
@@ -916,7 +916,6 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, domain
 		"type":   string(challenge.Type),
 		"result": string(challenge.Status),
 	}).Observe(time.Since(vStart).Seconds())
-	va.stats.TimingDuration(fmt.Sprintf("Validations.%s.%s", challenge.Type, challenge.Status), time.Since(vStart))
 
 	va.log.AuditObject("Validation result", logEvent)
 	va.log.Info(fmt.Sprintf("Validations: %+v", authz))
diff --git a/va/va_test.go b/va/va_test.go
index f9f51748f..be229fb63 100644
--- a/va/va_test.go
+++ b/va/va_test.go
@@ -27,6 +27,7 @@ import (
 
 	"github.com/golang/mock/gomock"
 	"github.com/jmhodges/clock"
+	"github.com/prometheus/client_golang/prometheus"
 	"golang.org/x/net/context"
 	"gopkg.in/square/go-jose.v2"
 
@@ -789,28 +790,22 @@ func TestValidateTLSSNI01NotSane(t *testing.T) {
 func TestPerformValidationInvalid(t *testing.T) {
 	va, _ := setup(nil, 0)
 
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-	mockScope := mock_metrics.NewMockScope(ctrl)
-	va.stats = mockScope
-	mockScope.EXPECT().TimingDuration("Validations.dns-01.invalid", gomock.Any())
-	mockScope.EXPECT().Inc(gomock.Any(), gomock.Any()).AnyTimes()
-
 	chalDNS := createChallenge(core.ChallengeTypeDNS01)
 	_, prob := va.PerformValidation(context.Background(), "foo.com", chalDNS, core.Authorization{})
 	test.Assert(t, prob != nil, "validation succeeded")
+
+	samples := test.CountHistogramSamples(va.metrics.validationTime.With(prometheus.Labels{
+		"type":   "dns-01",
+		"result": "invalid",
+	}))
+	if samples != 1 {
+		t.Errorf("Wrong number of samples for invalid validation. Expected 1, got %d", samples)
+	}
 }
 
 func TestDNSValidationEmpty(t *testing.T) {
 	va, _ := setup(nil, 0)
 
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-	mockScope := mock_metrics.NewMockScope(ctrl)
-	va.stats = mockScope
-	mockScope.EXPECT().TimingDuration("Validations.dns-01.invalid", gomock.Any())
-	mockScope.EXPECT().Inc(gomock.Any(), gomock.Any()).AnyTimes()
-
 	chalDNS := createChallenge(core.ChallengeTypeDNS01)
 	_, prob := va.PerformValidation(
 		context.Background(),
@@ -818,24 +813,33 @@ func TestDNSValidationEmpty(t *testing.T) {
 		chalDNS,
 		core.Authorization{})
 	test.AssertEquals(t, prob.Error(), "unauthorized :: No TXT records found for DNS challenge")
+
+	samples := test.CountHistogramSamples(va.metrics.validationTime.With(prometheus.Labels{
+		"type":   "dns-01",
+		"result": "invalid",
+	}))
+	if samples != 1 {
+		t.Errorf("Wrong number of samples for invalid validation. Expected 1, got %d", samples)
+	}
 }
 
 func TestPerformValidationValid(t *testing.T) {
 	va, _ := setup(nil, 0)
 
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
-	mockScope := mock_metrics.NewMockScope(ctrl)
-	va.stats = mockScope
-	mockScope.EXPECT().TimingDuration("Validations.dns-01.valid", gomock.Any())
-	mockScope.EXPECT().Inc(gomock.Any(), gomock.Any()).AnyTimes()
-
 	// create a challenge with well known token
 	chalDNS := core.DNSChallenge01()
 	chalDNS.Token = expectedToken
 	chalDNS.ProvidedKeyAuthorization = expectedKeyAuthorization
 	_, prob := va.PerformValidation(context.Background(), "good-dns01.com", chalDNS, core.Authorization{})
 	test.Assert(t, prob == nil, fmt.Sprintf("validation failed: %#v", prob))
+
+	samples := test.CountHistogramSamples(va.metrics.validationTime.With(prometheus.Labels{
+		"type":   "dns-01",
+		"result": "valid",
+	}))
+	if samples != 1 {
+		t.Errorf("Wrong number of samples for successful validation. Expected 1, got %d", samples)
+	}
 }
 
 func TestDNSValidationFailure(t *testing.T) {