Repository: google/certificate-transparency-go
Pull request #607 (2019-09-17 17:02:02)

Enforce http-error code on invalid SCT response for the metric

codecov[bot]: # [Codecov](https://codecov.io/gh/google/certificate-transparency-go/pull/607?src=pr&el=h1) Report
> Merging [#607](https://codecov.io/gh/google/certificate-transparency-go/pull/607?src=pr&el=desc) into [master](https://codecov.io/gh/google/certificate-transparency-go/commit/496c2e82955b5ae3cde7f00c5a2a902bc24366a0?src=pr&el=desc) will **increase** coverage by `0.02%`.
> The diff coverage is `0%`.

[![Impacted file tree graph](https://codecov.io/gh/google/certificate-transparency-go/pull/607/graphs/tree.svg?width=650&token=kYPnjCLppA&height=150&src=pr)](https://codecov.io/gh/google/certificate-transparency-go/pull/607?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #607      +/-   ##
==========================================
+ Coverage   71.83%   71.85%   +0.02%     
==========================================
  Files          90       90              
  Lines        9774     9776       +2     
==========================================
+ Hits         7021     7025       +4     
  Misses       2260     2260              
+ Partials      493      491       -2
```


| [Impacted Files](https://codecov.io/gh/google/certificate-transparency-go/pull/607?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [submission/distributor.go](https://codecov.io/gh/google/certificate-transparency-go/pull/607/diff?src=pr&el=tree#diff-c3VibWlzc2lvbi9kaXN0cmlidXRvci5nbw==) | `81.46% <0%> (-0.93%)` | :arrow_down: |
| [submission/races.go](https://codecov.io/gh/google/certificate-transparency-go/pull/607/diff?src=pr&el=tree#diff-c3VibWlzc2lvbi9yYWNlcy5nbw==) | `90.83% <0%> (+3.05%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/google/certificate-transparency-go/pull/607?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/google/certificate-transparency-go/pull/607?src=pr&el=footer). Last update [496c2e8...577beb0](https://codecov.io/gh/google/certificate-transparency-go/pull/607?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Mercurrent: Let's separate error-counting then
Mercurrent: PTAL
Mercurrent: PTAL

Patch:
diff --git a/submission/distributor.go b/submission/distributor.go
index d0c054da..ce164d9c 100644
--- a/submission/distributor.go
+++ b/submission/distributor.go
@@ -39,6 +39,7 @@ var (
 	distOnce      sync.Once
 	reqsCounter   monitoring.Counter   // logurl, ep => value
 	rspsCounter   monitoring.Counter   // logurl, ep, sc => value
+	errCounter    monitoring.Counter   // logurl, ep, status => value
 	logRspLatency monitoring.Histogram // logurl, ep => value
 )
 
@@ -46,6 +47,7 @@ var (
 func distInitMetrics(mf monitoring.MetricFactory) {
 	reqsCounter = mf.NewCounter("http_reqs", "Number of requests", "logurl", "ep")
 	rspsCounter = mf.NewCounter("http_rsps", "Number of responses", "logurl", "ep", "httpstatus")
+	errCounter = mf.NewCounter("err_count", "Number of errors", "logurl", "ep", "errtype")
 	logRspLatency = mf.NewHistogram("http_log_latency", "Latency of responses in seconds", "logurl", "ep")
 }
 
@@ -182,6 +184,23 @@ func incRspsCounter(logURL string, endpoint string, rspErr error) {
 	rspsCounter.Inc(logURL, endpoint, strconv.Itoa(status))
 }
 
+// incErrCounter increments corresponding errCounter if any error occurred during
+// submission to a Log.
+func incErrCounter(logURL string, endpoint string, rspErr error) {
+	if rspErr == nil {
+		return
+	}
+	err, ok := rspErr.(client.RspError)
+	switch {
+	case !ok:
+		errCounter.Inc(logURL, endpoint, "unknown_error")
+	case err.Err != nil && err.StatusCode == http.StatusOK:
+		errCounter.Inc(logURL, endpoint, "invalid_sct")
+	case err.Err != nil: // err.StatusCode != http.StatusOK.
+		errCounter.Inc(logURL, endpoint, "connection_error")
+	}
+}
+
 // SubmitToLog implements Submitter interface.
 func (d *Distributor) SubmitToLog(ctx context.Context, logURL string, chain []ct.ASN1Cert, asPreChain bool) (*ct.SignedCertificateTimestamp, error) {
 	lc, ok := d.logClients[logURL]
@@ -205,6 +224,7 @@ func (d *Distributor) SubmitToLog(ctx context.Context, logURL string, chain []ct
 	}
 	sct, err := addChain(ctx, chain)
 	incRspsCounter(logURL, endpoint, err)
+	incErrCounter(logURL, endpoint, err)
 	return sct, err
 }
 
diff --git a/submission/loglist_manager.go b/submission/loglist_manager.go
index 64d18ddc..68b14df5 100644
--- a/submission/loglist_manager.go
+++ b/submission/loglist_manager.go
@@ -21,8 +21,19 @@ import (
 	"time"
 
 	"github.com/google/certificate-transparency-go/schedule"
+	"github.com/google/trillian/monitoring"
 )
 
+var (
+	logRefOnce         sync.Once
+	logListLastRefresh monitoring.Gauge // Unix time
+)
+
+// logRefInitMetrics initializes all the exported metrics.
+func logRefInitMetrics(ctx context.Context, mf monitoring.MetricFactory) {
+	logListLastRefresh = mf.NewGauge("log_list_last_refresh", "Timestamp for last successful Log-list refresh")
+}
+
 // LogListManager runs loglist updates and keeps two latest versions of Log
 // list.
 type LogListManager struct {
@@ -35,14 +46,20 @@ type LogListManager struct {
 	latestLL   *LogListData
 	previousLL *LogListData
 	mu         sync.Mutex // guards latestLL and previousLL
+
+	mtf monitoring.MetricFactory
 }
 
 // NewLogListManager creates and inits a LogListManager instance.
-func NewLogListManager(llr LogListRefresher) *LogListManager {
+func NewLogListManager(llr LogListRefresher, mf monitoring.MetricFactory) *LogListManager {
+	if mf == nil {
+		mf = monitoring.InertMetricFactory{}
+	}
 	return &LogListManager{
 		Errors:    make(chan error, 1),
 		LLUpdates: make(chan LogListData, 1),
 		llr:       llr,
+		mtf:       mf,
 	}
 }
 
@@ -52,6 +69,7 @@ func NewLogListManager(llr LogListRefresher) *LogListManager {
 func (llm *LogListManager) Run(ctx context.Context, llRefresh time.Duration) {
 	llm.llRefreshInterval = llRefresh
 	go schedule.Every(ctx, llm.llRefreshInterval, llm.refreshLogListAndNotify)
+	logRefOnce.Do(func() { logRefInitMetrics(ctx, llm.mtf) })
 }
 
 // refreshLogListAndNotify runs single Log-list refresh and propagates data and
@@ -80,6 +98,7 @@ func (llm *LogListManager) RefreshLogList(ctx context.Context) (*LogListData, er
 	if err != nil {
 		return nil, err
 	}
+	logListLastRefresh.Set(float64(time.Now().Unix()))
 	if ll == nil {
 		// No updates
 		return nil, nil
diff --git a/submission/loglist_refresher.go b/submission/loglist_refresher.go
index 36179883..d199474b 100644
--- a/submission/loglist_refresher.go
+++ b/submission/loglist_refresher.go
@@ -66,10 +66,7 @@ func NewCustomLogListRefresher(client *http.Client, llPath string) LogListRefres
 // NewLogListRefresher creates and inits a LogListRefresherImpl instance using
 // default http.Client
 func NewLogListRefresher(llPath string) LogListRefresher {
-	return &logListRefresherImpl{
-		path:   llPath,
-		client: &http.Client{Timeout: httpClientTimeout},
-	}
+	return NewCustomLogListRefresher(&http.Client{Timeout: httpClientTimeout}, llPath)
 }
 
 // Refresh fetches the log list and returns its source, formed LogList and
diff --git a/submission/proxy_server.go b/submission/proxy_server.go
index e2cde5d0..da8acfc3 100644
--- a/submission/proxy_server.go
+++ b/submission/proxy_server.go
@@ -39,7 +39,7 @@ type ProxyServer struct {
 // NewProxyServer creates ProxyServer instance. Call Run() to init.
 func NewProxyServer(logListPath string, dBuilder DistributorBuilder, reqTimeout time.Duration, mf monitoring.MetricFactory) *ProxyServer {
 	s := &ProxyServer{addTimeout: reqTimeout}
-	s.p = NewProxy(NewLogListManager(NewLogListRefresher(logListPath)), dBuilder, mf)
+	s.p = NewProxy(NewLogListManager(NewLogListRefresher(logListPath), mf), dBuilder, mf)
 	return s
 }
 


Test patch:
diff --git a/submission/loglist_manager_test.go b/submission/loglist_manager_test.go
index 5fa15557..3eff46eb 100644
--- a/submission/loglist_manager_test.go
+++ b/submission/loglist_manager_test.go
@@ -23,10 +23,11 @@ import (
 	"time"
 
 	"github.com/google/certificate-transparency-go/testdata"
+	"github.com/google/trillian/monitoring"
 )
 
 func TestNoLLRefresher(t *testing.T) {
-	llm := NewLogListManager(nil)
+	llm := NewLogListManager(nil, nil)
 	_, err := llm.RefreshLogList(context.Background())
 	if err == nil {
 		t.Errorf("llm.RefreshLogList() on nil LogListRefresher expected to get error, got none")
@@ -34,7 +35,7 @@ func TestNoLLRefresher(t *testing.T) {
 }
 
 func TestNoLLRefresherAfterRun(t *testing.T) {
-	llm := NewLogListManager(nil)
+	llm := NewLogListManager(nil, nil)
 	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
 	defer cancel()
 
@@ -55,7 +56,7 @@ func TestFirstRefresh(t *testing.T) {
 	defer os.Remove(f)
 
 	llr := NewLogListRefresher(f)
-	llm := NewLogListManager(llr)
+	llm := NewLogListManager(llr, nil)
 	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
 	defer cancel()
 
@@ -78,7 +79,7 @@ func TestSecondRefresh(t *testing.T) {
 	defer os.Remove(f)
 
 	llr := NewLogListRefresher(f)
-	llm := NewLogListManager(llr)
+	llm := NewLogListManager(llr, monitoring.InertMetricFactory{})
 	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
 	defer cancel()
 	halfCtx, halfCancel := context.WithTimeout(context.Background(), time.Second)
diff --git a/submission/proxy_test.go b/submission/proxy_test.go
index 5471c594..e3f76014 100644
--- a/submission/proxy_test.go
+++ b/submission/proxy_test.go
@@ -48,7 +48,7 @@ func (llr stubLogListRefresher) Source() string {
 }
 
 func stubLogListManager() *LogListManager {
-	return NewLogListManager(stubLogListRefresher{})
+	return NewLogListManager(stubLogListRefresher{}, nil)
 }
 
 var imf monitoring.InertMetricFactory
@@ -100,7 +100,7 @@ func TestProxyInitState(t *testing.T) {
 	defer os.Remove(f)
 
 	llr := NewLogListRefresher(f)
-	p := NewProxy(NewLogListManager(llr), GetDistributorBuilder(ChromeCTPolicy, buildStubNoRootsLogClient, imf), imf)
+	p := NewProxy(NewLogListManager(llr, nil), GetDistributorBuilder(ChromeCTPolicy, buildStubNoRootsLogClient, imf), imf)
 	p.Run(context.Background(), 100*time.Millisecond, time.Hour)
 
 	ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)