algod : deregister metrics tracker on close (#3661)

## Summary

Metrics counters where not cleared on close and lead to duplicate entries in metering report.

## Test Plan

Added unit test. Tested manually.

Closes #3354

5 files changed, 94 insertions, 3 deletions

diff --git a/ledger/metrics.go b/ledger/metrics.go
index c7ac7dc00..de4cd6aed 100644
--- a/ledger/metrics.go
+++ b/ledger/metrics.go
@@ -40,6 +40,18 @@ func (mt *metricsTracker) loadFromDisk(l ledgerForTracker, _ basics.Round) error
 }
 
 func (mt *metricsTracker) close() {
+	if mt.ledgerTransactionsTotal != nil {
+		mt.ledgerTransactionsTotal.Deregister(nil)
+		mt.ledgerTransactionsTotal = nil
+	}
+	if mt.ledgerRewardClaimsTotal != nil {
+		mt.ledgerRewardClaimsTotal.Deregister(nil)
+		mt.ledgerRewardClaimsTotal = nil
+	}
+	if mt.ledgerRound != nil {
+		mt.ledgerRound.Deregister(nil)
+		mt.ledgerRound = nil
+	}
 }
 
 func (mt *metricsTracker) newBlock(blk bookkeeping.Block, delta ledgercore.StateDelta) {
diff --git a/ledger/metrics_test.go b/ledger/metrics_test.go
new file mode 100644
index 000000000..ca24e0f0f
--- /dev/null
+++ b/ledger/metrics_test.go
@@ -0,0 +1,76 @@
+// Copyright (C) 2019-2022 Algorand, Inc.
+// This file is part of go-algorand
+//
+// go-algorand is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as
+// published by the Free Software Foundation, either version 3 of the
+// License, or (at your option) any later version.
+//
+// go-algorand is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with go-algorand.  If not, see <https://www.gnu.org/licenses/>.
+
+package ledger
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/algorand/go-algorand/data/basics"
+	"github.com/algorand/go-algorand/data/bookkeeping"
+	"github.com/algorand/go-algorand/ledger/ledgercore"
+	ledgertesting "github.com/algorand/go-algorand/ledger/testing"
+	"github.com/algorand/go-algorand/protocol"
+	"github.com/algorand/go-algorand/test/partitiontest"
+	"github.com/algorand/go-algorand/util/metrics"
+)
+
+func TestMetricsReload(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	mt := metricsTracker{}
+	accts := ledgertesting.RandomAccounts(1, true)
+	ml := makeMockLedgerForTracker(t, true, 1, protocol.ConsensusCurrentVersion, []map[basics.Address]basics.AccountData{accts})
+
+	mt.loadFromDisk(ml, 0)
+	blk := bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: 1}}
+	mt.newBlock(blk, ledgercore.StateDelta{})
+	mt.close()
+
+	mt.loadFromDisk(ml, 0)
+	blk = bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: 2}}
+	mt.newBlock(blk, ledgercore.StateDelta{})
+
+	var buf strings.Builder
+	metrics.DefaultRegistry().WriteMetrics(&buf, "")
+	lines := strings.Split(buf.String(), "\n")
+	txCount := 0
+	rcCount := 0
+	rCount := 0
+	for _, line := range lines {
+		if strings.HasPrefix(line, "# HELP") || strings.HasPrefix(line, "# TYPE") {
+			// ignore comments
+			continue
+		}
+		if strings.HasPrefix(line, metrics.LedgerTransactionsTotal.Name) {
+			txCount++
+		}
+		if strings.HasPrefix(line, metrics.LedgerRewardClaimsTotal.Name) {
+			rcCount++
+		}
+		if strings.HasPrefix(line, metrics.LedgerRound.Name) {
+			rCount++
+		}
+	}
+	require.Equal(t, 1, txCount)
+	require.Equal(t, 1, rcCount)
+	require.Equal(t, 1, rCount)
+
+	mt.close()
+}
diff --git a/util/metrics/counter.go b/util/metrics/counter.go
index 3a7b36357..1fb33f67e 100644
--- a/util/metrics/counter.go
+++ b/util/metrics/counter.go
@@ -63,7 +63,7 @@ func (counter *Counter) Deregister(reg *Registry) {
 // Inc increases counter by 1
 // Much faster if labels is nil or empty.
 func (counter *Counter) Inc(labels map[string]string) {
-	if labels == nil || len(labels) == 0 {
+	if len(labels) == 0 {
 		counter.fastAddUint64(1)
 	} else {
 		counter.Add(1.0, labels)
@@ -98,7 +98,7 @@ func (counter *Counter) Add(x float64, labels map[string]string) {
 // If labels is nil this is much faster than Add()
 // Calls through to Add() if labels is not nil.
 func (counter *Counter) AddUint64(x uint64, labels map[string]string) {
-	if labels == nil || len(labels) == 0 {
+	if len(labels) == 0 {
 		counter.fastAddUint64(x)
 	} else {
 		counter.Add(float64(x), labels)
@@ -108,7 +108,7 @@ func (counter *Counter) AddUint64(x uint64, labels map[string]string) {
 // AddMicrosecondsSince increases counter by microseconds between Time t and now.
 // Fastest if labels is nil
 func (counter *Counter) AddMicrosecondsSince(t time.Time, labels map[string]string) {
-	counter.AddUint64(uint64(time.Now().Sub(t).Microseconds()), labels)
+	counter.AddUint64(uint64(time.Since(t).Microseconds()), labels)
 }
 
 func (counter *Counter) fastAddUint64(x uint64) {
diff --git a/util/metrics/registry.go b/util/metrics/registry.go
index d82b49367..33b902da3 100644
--- a/util/metrics/registry.go
+++ b/util/metrics/registry.go
@@ -50,6 +50,8 @@ func (r *Registry) Register(metric Metric) {
 
 // Deregister removes the given metric to the registry
 func (r *Registry) Deregister(metric Metric) {
+	r.metricsMu.Lock()
+	defer r.metricsMu.Unlock()
 	for i, m := range r.metrics {
 		if m == metric {
 			r.metrics = append(r.metrics[:i], r.metrics[i+1:]...)
diff --git a/util/metrics/service.go b/util/metrics/service.go
index 576d14404..e37f5c734 100644
--- a/util/metrics/service.go
+++ b/util/metrics/service.go
@@ -37,6 +37,7 @@ var (
 var (
 	// the duration of which we'll keep a metric in-memory and keep reporting it.
 	// when a metric time expires, it would get removed.
+	// TODO: implement or remove
 	maxMetricRetensionDuration = time.Duration(5) * time.Minute
 )