letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Use fields, not globals, for stats (#2790) 

Following up on #2752, we don't need to use global vars for our Prometheus stats. We already have a custom registry plumbed through using Scope objects. In this PR, expose the MustRegister method of that registry through the Scope interface, and move existing global vars to be fields of objects. This should improve testability somewhat.

Note that this has a bit of an unfortunate side effect: two instances of the same stats-using class (e.g. VA) can't use the same Scope object, because their MustRegister calls will conflict. In practice this is fine since we never instantiate duplicates of the the classes that use stats, but it's something we should keep an eye on.

Updates #2733
This commit is contained in:
Jacob Hoffman-Andrews 2017-06-06 12:09:31 -07:00 committed by Roland Bracewell Shoemaker
parent 589d383923
commit 0bfb542514
5 changed files with 104 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
ca/ca.go Executable file → Normal file
View File

@ -98,19 +98,6 @@ const (
metricCSRExtensionOther = "CSRExtensions.Other"
)
var (
signatureCount = prometheus.NewCounterVec(
prometheus.CounterOpts{
Name: "signatures",
Help: "Number of signatures",
},
[]string{"purpose"})
)
func init() {
prometheus.MustRegister(signatureCount)
}
type certificateStorage interface {
AddCertificate(context.Context, []byte, int64, []byte) (string, error)
}
@ -135,6 +122,7 @@ type CertificateAuthorityImpl struct {
maxNames int
forceCNFromSAN bool
enableMustStaple bool
signatureCount *prometheus.CounterVec
}
// Issuer represents a single issuer certificate, along with its key.
@ -238,6 +226,14 @@ func NewCertificateAuthorityImpl(
return nil, errors.New("must specify rsaProfile and ecdsaProfile")
}
signatureCount := prometheus.NewCounterVec(
prometheus.CounterOpts{
Name: "signatures",
Help: "Number of signatures",
},
[]string{"purpose"})
stats.MustRegister(signatureCount)
ca = &CertificateAuthorityImpl{
issuers: internalIssuers,
defaultIssuer: defaultIssuer,
@ -250,6 +246,7 @@ func NewCertificateAuthorityImpl(
keyPolicy: keyPolicy,
forceCNFromSAN: !config.DoNotForceCN, // Note the inversion here
enableMustStaple: config.EnableMustStaple,
signatureCount: signatureCount,
}
if config.Expiry == "" {
@ -378,7 +375,7 @@ func (ca *CertificateAuthorityImpl) GenerateOCSP(ctx context.Context, xferObj co
ocspResponse, err := issuer.ocspSigner.Sign(signRequest)
ca.noteSignError(err)
if err == nil {
signatureCount.With(prometheus.Labels{"purpose": "ocsp"}).Inc()
ca.signatureCount.With(prometheus.Labels{"purpose": "ocsp"}).Inc()
}
return ocspResponse, err
}
@ -473,7 +470,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(ctx context.Context, csr x5
ca.log.AuditErr(fmt.Sprintf("Signing failed: serial=[%s] err=[%v]", serialHex, err))
return emptyCert, err
}
signatureCount.With(prometheus.Labels{"purpose": "cert"}).Inc()
ca.signatureCount.With(prometheus.Labels{"purpose": "cert"}).Inc()
if len(certPEM) == 0 {
err = berrors.InternalServerError("no certificate returned by server")

18
ca/ca_test.go
View File

@ -674,6 +674,8 @@ func TestExtensions(t *testing.T) {
defer ctrl.Finish()
stats := mock_metrics.NewMockScope(ctrl)
stats.EXPECT().MustRegister(gomock.Any()).AnyTimes()
ca, err := NewCertificateAuthorityImpl(
testCtx.caConfig,
testCtx.fc,
@ -697,7 +699,7 @@ func TestExtensions(t *testing.T) {
test.AssertNotError(t, err, "Error parsing UnsupportedExtensionCSR")
sign := func(csr *x509.CertificateRequest) *x509.Certificate {
signatureCount.Reset()
ca.signatureCount.Reset()
coreCert, err := ca.IssueCertificate(ctx, *csr, 1001)
test.AssertNotError(t, err, "Failed to issue")
cert, err := x509.ParseCertificate(coreCert.DER)
@ -709,7 +711,7 @@ func TestExtensions(t *testing.T) {
// Must Staple.
stats.EXPECT().Inc(metricCSRExtensionTLSFeature, int64(1)).Return(nil)
noStapleCert := sign(mustStapleCSR)
test.AssertEquals(t, signatureCountByPurpose("cert"), 1)
test.AssertEquals(t, signatureCountByPurpose("cert", ca.signatureCount), 1)
test.AssertEquals(t, countMustStaple(t, noStapleCert), 0)
// With ca.enableMustStaple = true, a TLS feature extension should put a must-staple
@ -717,21 +719,21 @@ func TestExtensions(t *testing.T) {
ca.enableMustStaple = true
stats.EXPECT().Inc(metricCSRExtensionTLSFeature, int64(1)).Return(nil)
singleStapleCert := sign(mustStapleCSR)
test.AssertEquals(t, signatureCountByPurpose("cert"), 1)
test.AssertEquals(t, signatureCountByPurpose("cert", ca.signatureCount), 1)
test.AssertEquals(t, countMustStaple(t, singleStapleCert), 1)
// Even if there are multiple TLS Feature extensions, only one extension should be included
stats.EXPECT().Inc(metricCSRExtensionTLSFeature, int64(1)).Return(nil)
duplicateMustStapleCert := sign(duplicateMustStapleCSR)
test.AssertEquals(t, signatureCountByPurpose("cert"), 1)
test.AssertEquals(t, signatureCountByPurpose("cert", ca.signatureCount), 1)
test.AssertEquals(t, countMustStaple(t, duplicateMustStapleCert), 1)
// ... but if it doesn't ask for stapling, there should be an error
stats.EXPECT().Inc(metricCSRExtensionTLSFeature, int64(1)).Return(nil)
stats.EXPECT().Inc(metricCSRExtensionTLSFeatureInvalid, int64(1)).Return(nil)
signatureCount.Reset()
ca.signatureCount.Reset()
_, err = ca.IssueCertificate(ctx, *tlsFeatureUnknownCSR, 1001)
test.AssertEquals(t, signatureCountByPurpose("cert"), 0)
test.AssertEquals(t, signatureCountByPurpose("cert", ca.signatureCount), 0)
test.AssertError(t, err, "Allowed a CSR with an empty TLS feature extension")
test.Assert(t, berrors.Is(err, berrors.Malformed), "Wrong error type when rejecting a CSR with empty TLS feature extension")
@ -739,11 +741,11 @@ func TestExtensions(t *testing.T) {
// extensions as the TLS Feature cert above, minus the TLS Feature Extension
stats.EXPECT().Inc(metricCSRExtensionOther, int64(1)).Return(nil)
unsupportedExtensionCert := sign(unsupportedExtensionCSR)
test.AssertEquals(t, signatureCountByPurpose("cert"), 1)
test.AssertEquals(t, signatureCountByPurpose("cert", ca.signatureCount), 1)
test.AssertEquals(t, len(unsupportedExtensionCert.Extensions), len(singleStapleCert.Extensions)-1)
}
func signatureCountByPurpose(signatureType string) int {
func signatureCountByPurpose(signatureType string, signatureCount *prometheus.CounterVec) int {
ch := make(chan prometheus.Metric, 10)
signatureCount.With(prometheus.Labels{"purpose": signatureType}).Collect(ch)
m := <-ch

13
metrics/mock_metrics/mock_scope.go
View File

@ -6,6 +6,7 @@ package mock_metrics
import (
gomock "github.com/golang/mock/gomock"
metrics "github.com/letsencrypt/boulder/metrics"
prometheus "github.com/prometheus/client_golang/prometheus"
time "time"
)
@ -60,6 +61,18 @@ func (_mr *_MockScopeRecorder) Inc(arg0, arg1 interface{}) *gomock.Call {
return _mr.mock.ctrl.RecordCall(_mr.mock, "Inc", arg0, arg1)
}
func (_m *MockScope) MustRegister(_param0 ...prometheus.Collector) {
_s := []interface{}{}
for _, _x := range _param0 {
_s = append(_s, _x)
}
_m.ctrl.Call(_m, "MustRegister", _s...)
}
func (_mr *_MockScopeRecorder) MustRegister(arg0 ...interface{}) *gomock.Call {
return _mr.mock.ctrl.RecordCall(_mr.mock, "MustRegister", arg0...)
}
func (_m *MockScope) NewScope(_param0 ...string) metrics.Scope {
_s := []interface{}{}
for _, _x := range _param0 {

44
metrics/scope.go
View File

@ -18,13 +18,15 @@ type Scope interface {
Timing(stat string, delta int64) error
TimingDuration(stat string, delta time.Duration) error
SetInt(stat string, value int64) error
MustRegister(...prometheus.Collector)
}
// promScope is a Scope that sends data to Prometheus
type promScope struct {
prometheus.Registerer
*autoRegisterer
prefix string
registerer prometheus.Registerer
prefix string
}
var _ Scope = &promScope{}
@ -32,21 +34,17 @@ var _ Scope = &promScope{}
// NewPromScope returns a Scope that sends data to Prometheus
func NewPromScope(registerer prometheus.Registerer, scopes ...string) Scope {
return &promScope{
Registerer: registerer,
prefix: strings.Join(scopes, ".") + ".",
autoRegisterer: newAutoRegisterer(registerer),
}
}
// NewNoopScope returns a Scope that won't collect anything
func NewNoopScope() Scope {
return NewPromScope(prometheus.NewRegistry())
}
// NewScope generates a new Scope prefixed by this Scope's prefix plus the
// prefixes given joined by periods
func (s *promScope) NewScope(scopes ...string) Scope {
scope := strings.Join(scopes, ".")
return NewPromScope(s.registerer, s.prefix+scope)
return NewPromScope(s.Registerer, s.prefix+scope)
}
// Inc increments the given stat and adds the Scope's prefix to the name
@ -85,3 +83,33 @@ func (s *promScope) SetInt(stat string, value int64) error {
s.autoGauge(s.prefix + stat).Set(float64(value))
return nil
}
type noopScope struct{}
// NewNoopScope returns a Scope that won't collect anything
func NewNoopScope() Scope {
return noopScope{}
}
func (ns noopScope) NewScope(scopes ...string) Scope {
return ns
}
func (_ noopScope) Inc(stat string, value int64) error {
return nil
}
func (_ noopScope) Gauge(stat string, value int64) error {
return nil
}
func (_ noopScope) GaugeDelta(stat string, value int64) error {
return nil
}
func (_ noopScope) Timing(stat string, delta int64) error {
return nil
}
func (_ noopScope) TimingDuration(stat string, delta time.Duration) error {
return nil
}
func (_ noopScope) SetInt(stat string, value int64) error {
return nil
}
func (_ noopScope) MustRegister(...prometheus.Collector) {
}

69
va/va.go
View File

@ -43,34 +43,22 @@ const (
maxResponseSize = 128
)
var (
validationTime = prometheus.NewHistogramVec(
prometheus.HistogramOpts{
Name: "validation_time",
Help: "Time taken to validate a challenge",
},
[]string{"type", "result"})
)
func init() {
prometheus.MustRegister(validationTime)
}
var validationTimeout = time.Second * 5
// ValidationAuthorityImpl represents a VA
type ValidationAuthorityImpl struct {
log blog.Logger
dnsResolver bdns.DNSResolver
issuerDomain string
safeBrowsing SafeBrowsing
httpPort int
httpsPort int
tlsPort int
userAgent string
stats metrics.Scope
clk clock.Clock
caaDR *cdr.CAADistributedResolver
log blog.Logger
dnsResolver bdns.DNSResolver
issuerDomain string
safeBrowsing SafeBrowsing
httpPort int
httpsPort int
tlsPort int
userAgent string
stats metrics.Scope
clk clock.Clock
caaDR *cdr.CAADistributedResolver
validationTime *prometheus.HistogramVec
}
// NewValidationAuthorityImpl constructs a new VA
@ -85,18 +73,27 @@ func NewValidationAuthorityImpl(
clk clock.Clock,
logger blog.Logger,
) *ValidationAuthorityImpl {
validationTime := prometheus.NewHistogramVec(
prometheus.HistogramOpts{
Name: "validation_time",
Help: "Time taken to validate a challenge",
},
[]string{"type", "result"})
stats.MustRegister(validationTime)
return &ValidationAuthorityImpl{
log: logger,
dnsResolver: resolver,
issuerDomain: issuerDomain,
safeBrowsing: sbc,
httpPort: pc.HTTPPort,
httpsPort: pc.HTTPSPort,
tlsPort: pc.TLSPort,
userAgent: userAgent,
stats: stats,
clk: clk,
caaDR: cdrClient,
log: logger,
dnsResolver: resolver,
issuerDomain: issuerDomain,
safeBrowsing: sbc,
httpPort: pc.HTTPPort,
httpsPort: pc.HTTPSPort,
tlsPort: pc.TLSPort,
userAgent: userAgent,
stats: stats,
clk: clk,
caaDR: cdrClient,
validationTime: validationTime,
}
}
@ -793,7 +790,7 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, domain
logEvent.Challenge = challenge
validationTime.With(prometheus.Labels{
va.validationTime.With(prometheus.Labels{
"type": string(challenge.Type),
"result": string(challenge.Status),
}).Observe(time.Since(vStart).Seconds())