diff --git a/.codecov.yml b/.codecov.yml index def78dc007..691bb6d2f4 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -1,10 +1,15 @@ codecov: require_ci_to_pass: yes +flags: + full_coverage: + joined: false + ignore: - "**/*_gen.go" - "**/*_gen_test.go" - "**/generated" + - "**/*_string.go" # ignore stringer-generated code coverage: status: diff --git a/.github/workflows/ci-nightly.yml b/.github/workflows/ci-nightly.yml index 049fa67d4f..577953ef5d 100644 --- a/.github/workflows/ci-nightly.yml +++ b/.github/workflows/ci-nightly.yml @@ -103,13 +103,15 @@ jobs: run: | ./scripts/configure_dev.sh PACKAGES="$(go list ./... | grep -v /go-algorand/test/)" - export PACKAGE_NAMES=$(echo $PACKAGES | tr -d '\n') + export PACKAGE_NAMES=$(echo $PACKAGES | tr -d '\n') + COVERPACKAGES="$(go list ./... | grep -E -v '/go-algorand/(test|debug|cmd|config/defaultsGenerator|tools)' | grep -E -v '(test|testing|mock|mocks)$' | paste -sd ',' -)" mkdir -p test_results/${{ matrix.platform }}_test_nightly/${PARTITION_ID} go tool -modfile=tool.mod gotestsum --format standard-quiet \ --junitfile ~/test_results/${{ matrix.platform }}_test_nightly/${PARTITION_ID}/results.xml \ --jsonfile ~/test_results/${{ matrix.platform }}_test_nightly/${PARTITION_ID}/testresults.json \ -- --tags "sqlite_unlock_notify sqlite_omit_load_extension" \ -race -timeout 1h -coverprofile=coverage.txt -covermode=atomic -p 1 \ + -coverpkg=$COVERPACKAGES \ $PACKAGE_NAMES - name: Notify Slack on failure if: failure() @@ -142,6 +144,7 @@ jobs: with: token: ${{ env.CODECOV_TOKEN }} file: ./coverage.txt + flags: full_coverage fail_ci_if_error: false - name: Upload test results to Codecov if: ${{ !cancelled() }} diff --git a/.github/workflows/ci-pr.yml b/.github/workflows/ci-pr.yml index 0a97bfab28..876215417a 100644 --- a/.github/workflows/ci-pr.yml +++ b/.github/workflows/ci-pr.yml @@ -4,6 +4,13 @@ on: branches: - master - 'rel/**' + workflow_dispatch: + inputs: + full_coverage: + description: 'Run with -coverpkg to measure cross-package coverage (slower)' + required: false + type: boolean + default: false env: CODECOV_TOKEN: "8b4a1f91-f154-4c26-b84c-c9aaa90159c6" # Same public token from CircleCI config @@ -49,13 +56,19 @@ jobs: - name: Run tests run: | PACKAGES="$(go list ./... | grep -v /go-algorand/test/)" - export PACKAGE_NAMES=$(echo $PACKAGES | tr -d '\n') + export PACKAGE_NAMES=$(echo $PACKAGES | tr -d '\n') + COVERPKG_FLAG="" + if [[ "${{ inputs.full_coverage }}" == "true" ]]; then + COVERPACKAGES="$(go list ./... | grep -E -v '/go-algorand/(test|debug|cmd|config/defaultsGenerator|tools)' | grep -E -v '(test|testing|mock|mocks)$' | paste -sd ',' -)" + COVERPKG_FLAG="-coverpkg=$COVERPACKAGES" + fi mkdir -p test_results/${{ matrix.platform }}_test/${PARTITION_ID} go tool -modfile=tool.mod gotestsum --format standard-quiet \ --junitfile ~/test_results/${{ matrix.platform }}_test/${PARTITION_ID}/results.xml \ --jsonfile ~/test_results/${{ matrix.platform }}_test/${PARTITION_ID}/testresults.json \ -- --tags "sqlite_unlock_notify sqlite_omit_load_extension" $SHORTTEST \ -race -timeout 1h -coverprofile=coverage.txt -covermode=atomic -p 4 \ + $COVERPKG_FLAG \ $PACKAGE_NAMES - name: Notify Slack on failure if: failure() @@ -89,6 +102,7 @@ jobs: with: token: ${{ env.CODECOV_TOKEN }} file: ./coverage.txt + flags: ${{ inputs.full_coverage && 'full_coverage' || '' }} fail_ci_if_error: false - name: Upload test results to Codecov if: ${{ !cancelled() }} diff --git a/Makefile b/Makefile index ee4099c1c2..e9552dcafc 100644 --- a/Makefile +++ b/Makefile @@ -84,6 +84,7 @@ GOLDFLAGS := $(GOLDFLAGS_BASE) \ -X github.com/algorand/go-algorand/config.Channel=$(CHANNEL) UNIT_TEST_SOURCES := $(sort $(shell GOPATH=$(GOPATH) && GO111MODULE=off && go list ./... | grep -v /go-algorand/test/ )) +COVERPKG_PACKAGES := $(sort $(shell GOPATH=$(GOPATH) && GO111MODULE=off && go list ./... | egrep -v '/go-algorand/(test|debug|cmd|config/defaultsGenerator|tools)' | egrep -v '(test|testing|mocks|mock)$$' )) ALGOD_API_PACKAGES := $(sort $(shell GOPATH=$(GOPATH) && GO111MODULE=off && cd daemon/algod/api; go list ./... )) GOMOD_DIRS := ./tools/block-generator ./tools/x-repo-types @@ -132,8 +133,19 @@ check_shell: sanity: fix lint fmt tidy modernize +# "make cover" runs all tests, and collects full coverage across all go-algorand packages by setting -coverpkg. +# Without setting -coverpkg, coverage reports only measure lines of code exercised within the same package as the tests. +# +# "make cover PACKAGE=X" runs all tests in package github.com/algorand/go-algorand/X/... and collects full coverage +# across all packages that are dependencies of that package. cover: - go test $(GOTAGS) -coverprofile=cover.out $(UNIT_TEST_SOURCES) +ifeq ($(PACKAGE),) + $(GOTESTCOMMAND) $(GOTAGS) -coverprofile=cover.out $(UNIT_TEST_SOURCES) -covermode=atomic -coverpkg=$(shell echo $(COVERPKG_PACKAGES) | sed 's/ /,/g') +else + cd $(PACKAGE); \ + $(GOTESTCOMMAND) $(GOTAGS) -coverprofile=cover.out ./... -covermode=atomic -coverpkg=$$( (go list -f '{{ join .Deps "\n" }}' ./...; go list -f '{{ join .TestImports "\n" }}' ./...) | grep 'github.com/algorand/go-algorand' | egrep -v '/go-algorand/(test|debug|cmd|config/defaultsGenerator|tools)' | egrep -v '(test|testing|mocks|mock)$$' | sort | uniq | paste -sd ',' -); \ + go tool cover -html cover.out +endif prof: cd node && go test $(GOTAGS) -cpuprofile=cpu.out -memprofile=mem.out -mutexprofile=mutex.out diff --git a/ledger/metrics.go b/ledger/metrics.go index aeae7e7853..52e3ebd0c6 100644 --- a/ledger/metrics.go +++ b/ledger/metrics.go @@ -31,31 +31,38 @@ type metricsTracker struct { ledgerRewardClaimsTotal *metrics.Counter ledgerRound *metrics.Gauge ledgerDBRound *metrics.Gauge + registry *metrics.Registry } func (mt *metricsTracker) loadFromDisk(l ledgerForTracker, _ basics.Round) error { - mt.ledgerTransactionsTotal = metrics.MakeCounter(metrics.LedgerTransactionsTotal) - mt.ledgerRewardClaimsTotal = metrics.MakeCounter(metrics.LedgerRewardClaimsTotal) - mt.ledgerRound = metrics.MakeGauge(metrics.LedgerRound) - mt.ledgerDBRound = metrics.MakeGauge(metrics.LedgerDBRound) + reg := mt.registry + mt.ledgerTransactionsTotal = metrics.MakeCounterUnregistered(metrics.LedgerTransactionsTotal) + mt.ledgerTransactionsTotal.Register(reg) + mt.ledgerRewardClaimsTotal = metrics.MakeCounterUnregistered(metrics.LedgerRewardClaimsTotal) + mt.ledgerRewardClaimsTotal.Register(reg) + mt.ledgerRound = metrics.MakeGaugeUnregistered(metrics.LedgerRound) + mt.ledgerRound.Register(reg) + mt.ledgerDBRound = metrics.MakeGaugeUnregistered(metrics.LedgerDBRound) + mt.ledgerDBRound.Register(reg) return nil } func (mt *metricsTracker) close() { + reg := mt.registry if mt.ledgerTransactionsTotal != nil { - mt.ledgerTransactionsTotal.Deregister(nil) + mt.ledgerTransactionsTotal.Deregister(reg) mt.ledgerTransactionsTotal = nil } if mt.ledgerRewardClaimsTotal != nil { - mt.ledgerRewardClaimsTotal.Deregister(nil) + mt.ledgerRewardClaimsTotal.Deregister(reg) mt.ledgerRewardClaimsTotal = nil } if mt.ledgerRound != nil { - mt.ledgerRound.Deregister(nil) + mt.ledgerRound.Deregister(reg) mt.ledgerRound = nil } if mt.ledgerDBRound != nil { - mt.ledgerDBRound.Deregister(nil) + mt.ledgerDBRound.Deregister(reg) mt.ledgerDBRound = nil } } diff --git a/ledger/metrics_test.go b/ledger/metrics_test.go index 51ba010fed..943b44fcf8 100644 --- a/ledger/metrics_test.go +++ b/ledger/metrics_test.go @@ -34,7 +34,11 @@ import ( func TestMetricsReload(t *testing.T) { partitiontest.PartitionTest(t) - mt := metricsTracker{} + // Use a non-default registry so the test is not affected by metrics + // registered by other packages when running with -coverpkg. + registry := metrics.MakeRegistry() + + mt := metricsTracker{registry: registry} accts := ledgertesting.RandomAccounts(1, true) ml := makeMockLedgerForTracker(t, true, 1, protocol.ConsensusCurrentVersion, []map[basics.Address]basics.AccountData{accts}) @@ -48,7 +52,7 @@ func TestMetricsReload(t *testing.T) { mt.newBlock(blk, ledgercore.StateDelta{}) var buf strings.Builder - metrics.DefaultRegistry().WriteMetrics(&buf, "") + registry.WriteMetrics(&buf, "") lines := strings.Split(buf.String(), "\n") txCount := 0 rcCount := 0 diff --git a/scripts/travis/upload_coverage.sh b/scripts/travis/upload_coverage.sh index 8ab3b56ea6..bdeee7e938 100755 --- a/scripts/travis/upload_coverage.sh +++ b/scripts/travis/upload_coverage.sh @@ -3,7 +3,7 @@ set -eo pipefail if [[ -z "$CODECOV_TOKEN" ]]; then - /usr/bin/env bash scripts/travis/codecov + /usr/bin/env bash scripts/travis/codecov "$@" else - /usr/bin/env bash scripts/travis/codecov -t $CODECOV_TOKEN + /usr/bin/env bash scripts/travis/codecov -t $CODECOV_TOKEN "$@" fi diff --git a/util/metrics/counter.go b/util/metrics/counter.go index 54781661f9..b2bbc951b0 100644 --- a/util/metrics/counter.go +++ b/util/metrics/counter.go @@ -28,14 +28,14 @@ type Counter struct { // MakeCounter create a new counter with the provided name and description. func MakeCounter(metric MetricName) *Counter { - c := makeCounter(metric) + c := MakeCounterUnregistered(metric) c.Register(nil) return c } -// makeCounter create a new counter with the provided name and description +// MakeCounterUnregistered creates a new counter with the provided name and description // but does not register it with the default registry. -func makeCounter(metric MetricName) *Counter { +func MakeCounterUnregistered(metric MetricName) *Counter { c := &Counter{c: couge{ values: make([]*cougeValues, 0), description: metric.Description, diff --git a/util/metrics/counter_test.go b/util/metrics/counter_test.go index 51e2d44691..9e28a49bd1 100644 --- a/util/metrics/counter_test.go +++ b/util/metrics/counter_test.go @@ -38,6 +38,9 @@ func TestMetricCounter(t *testing.T) { MetricTest: NewMetricTest(), } + // create a non-default registry for the metrics in this test + registry := MakeRegistry() + // create a http listener. port := test.createListener("127.0.0.1:0") @@ -46,10 +49,12 @@ func TestMetricCounter(t *testing.T) { Labels: map[string]string{ "host_name": "host_one", "session_id": "AFX-229"}, + registry: registry, }) metricService.Start(context.Background()) - counter := MakeCounter(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"}) + counter := MakeCounterUnregistered(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"}) + counter.Register(registry) for i := 0; i < 20; i++ { counter.Inc(map[string]string{"pid": "123", "data_host": fmt.Sprintf("host%d", i%5)}) @@ -61,7 +66,7 @@ func TestMetricCounter(t *testing.T) { metricService.Shutdown() - counter.Deregister(nil) + counter.Deregister(registry) // test the metrics values. test.Lock() @@ -84,6 +89,9 @@ func TestMetricCounterFastInts(t *testing.T) { MetricTest: NewMetricTest(), } + // create a non-default registry for the metrics in this test + registry := MakeRegistry() + // create a http listener. port := test.createListener("127.0.0.1:0") @@ -92,10 +100,12 @@ func TestMetricCounterFastInts(t *testing.T) { Labels: map[string]string{ "host_name": "host_one", "session_id": "AFX-229"}, + registry: registry, }) metricService.Start(context.Background()) - counter := MakeCounter(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"}) + counter := MakeCounterUnregistered(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"}) + counter.Register(registry) for i := 0; i < 20; i++ { counter.Inc(nil) @@ -108,7 +118,7 @@ func TestMetricCounterFastInts(t *testing.T) { metricService.Shutdown() - counter.Deregister(nil) + counter.Deregister(registry) // test the metrics values. test.Lock() @@ -131,6 +141,9 @@ func TestMetricCounterMixed(t *testing.T) { MetricTest: NewMetricTest(), } + // create a non-default registry for the metrics in this test + registry := MakeRegistry() + // create a http listener. port := test.createListener("127.0.0.1:0") @@ -139,10 +152,12 @@ func TestMetricCounterMixed(t *testing.T) { Labels: map[string]string{ "host_name": "host_one", "session_id": "AFX-229"}, + registry: registry, }) metricService.Start(context.Background()) - counter := MakeCounter(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"}) + counter := MakeCounterUnregistered(MetricName{Name: "metric_test_name1", Description: "this is the metric test for counter object"}) + counter.Register(registry) counter.AddUint64(5, nil) counter.AddUint64(8, map[string]string{}) @@ -157,7 +172,7 @@ func TestMetricCounterMixed(t *testing.T) { metricService.Shutdown() - counter.Deregister(nil) + counter.Deregister(registry) // test the metrics values. test.Lock() diff --git a/util/metrics/gauge.go b/util/metrics/gauge.go index eabf156720..bf7d3b6940 100644 --- a/util/metrics/gauge.go +++ b/util/metrics/gauge.go @@ -27,14 +27,14 @@ type Gauge struct { // MakeGauge create a new gauge with the provided name and description. func MakeGauge(metric MetricName) *Gauge { - c := makeGauge(metric) + c := MakeGaugeUnregistered(metric) c.Register(nil) return c } -// makeGauge create a new gauge with the provided name and description +// MakeGaugeUnregistered creates a new gauge with the provided name and description // but does not register it with the default registry. -func makeGauge(metric MetricName) *Gauge { +func MakeGaugeUnregistered(metric MetricName) *Gauge { c := &Gauge{g: couge{ values: make([]*cougeValues, 0), description: metric.Description, diff --git a/util/metrics/gauge_test.go b/util/metrics/gauge_test.go index 679c613088..8410e1f101 100644 --- a/util/metrics/gauge_test.go +++ b/util/metrics/gauge_test.go @@ -37,6 +37,10 @@ func TestMetricGauge(t *testing.T) { test := &GaugeTest{ MetricTest: NewMetricTest(), } + + // create a non-default registry for the metrics in this test + registry := MakeRegistry() + // create a http listener. port := test.createListener("127.0.0.1:0") @@ -45,11 +49,13 @@ func TestMetricGauge(t *testing.T) { Labels: map[string]string{ "host_name": "host_one", "session_id": "AFX-229"}, + registry: registry, }) metricService.Start(context.Background()) gauges := make([]*Gauge, 3) for i := 0; i < 3; i++ { - gauges[i] = MakeGauge(MetricName{Name: fmt.Sprintf("gauge_%d", i), Description: "this is the metric test for gauge object"}) + gauges[i] = MakeGaugeUnregistered(MetricName{Name: fmt.Sprintf("gauge_%d", i), Description: "this is the metric test for gauge object"}) + gauges[i].Register(registry) } for i := 0; i < 9; i++ { gauges[i%3].Set(uint64(i*100 + i)) @@ -62,7 +68,7 @@ func TestMetricGauge(t *testing.T) { metricService.Shutdown() for _, gauge := range gauges { - gauge.Deregister(nil) + gauge.Deregister(registry) } // test the metrics values. diff --git a/util/metrics/opencensus.go b/util/metrics/opencensus.go index 4bda632887..a15136937f 100644 --- a/util/metrics/opencensus.go +++ b/util/metrics/opencensus.go @@ -78,7 +78,7 @@ type statCounter struct { // WriteMetric outputs Prometheus metrics for all labels/values in statCounter func (st *statCounter) WriteMetric(buf *strings.Builder, parentLabels string) { name := sanitizePrometheusName(st.name) - counter := makeCounter(MetricName{name, st.description}) + counter := MakeCounterUnregistered(MetricName{name, st.description}) for i := 0; i < len(st.labels); i++ { counter.AddUint64(uint64(st.values[i]), st.labels[i]) } @@ -87,7 +87,7 @@ func (st *statCounter) WriteMetric(buf *strings.Builder, parentLabels string) { // AddMetric outputs all statCounter's labels/values into a map func (st *statCounter) AddMetric(values map[string]float64) { - counter := makeCounter(MetricName{st.name, st.description}) + counter := MakeCounterUnregistered(MetricName{st.name, st.description}) for i := 0; i < len(st.labels); i++ { counter.AddUint64(uint64(st.values[i]), st.labels[i]) } @@ -105,7 +105,7 @@ type statDistribution struct { // WriteMetric outputs Prometheus metrics for all labels/values in statCounter func (st *statDistribution) WriteMetric(buf *strings.Builder, parentLabels string) { name := sanitizePrometheusName(st.name) - gauge := makeGauge(MetricName{name, st.description}) + gauge := MakeGaugeUnregistered(MetricName{name, st.description}) for i := 0; i < len(st.labels); i++ { gauge.SetLabels(uint64(st.values[i]), st.labels[i]) } @@ -114,7 +114,7 @@ func (st *statDistribution) WriteMetric(buf *strings.Builder, parentLabels strin // AddMetric outputs all statCounter's labels/values into a map func (st *statDistribution) AddMetric(values map[string]float64) { - gauge := makeGauge(MetricName{st.name, st.description}) + gauge := MakeGaugeUnregistered(MetricName{st.name, st.description}) for i := 0; i < len(st.labels); i++ { gauge.SetLabels(uint64(st.values[i]), st.labels[i]) } diff --git a/util/metrics/prometheus.go b/util/metrics/prometheus.go index e916491fc4..d6fa5bae33 100644 --- a/util/metrics/prometheus.go +++ b/util/metrics/prometheus.go @@ -75,7 +75,7 @@ func collectPrometheusMetrics(names []string) []Metric { } if _, ok := namesMap[metric.GetName()]; len(namesMap) > 0 && ok || len(namesMap) == 0 { if metric.GetType() == iopc.MetricType_COUNTER && metric.GetMetric() != nil { - counter := makeCounter(MetricName{metric.GetName(), metric.GetHelp()}) + counter := MakeCounterUnregistered(MetricName{metric.GetName(), metric.GetHelp()}) ma := metric.GetMetric() for _, m := range ma { if m.GetCounter() == nil { @@ -87,7 +87,7 @@ func collectPrometheusMetrics(names []string) []Metric { } result = append(result, counter) } else if metric.GetType() == iopc.MetricType_GAUGE && metric.GetMetric() != nil { - gauge := makeGauge(MetricName{metric.GetName(), metric.GetHelp()}) + gauge := MakeGaugeUnregistered(MetricName{metric.GetName(), metric.GetHelp()}) ma := metric.GetMetric() for _, m := range ma { diff --git a/util/metrics/registry_test.go b/util/metrics/registry_test.go index c8251008f6..b8f91a67ed 100644 --- a/util/metrics/registry_test.go +++ b/util/metrics/registry_test.go @@ -27,15 +27,22 @@ import ( func TestWriteAdd(t *testing.T) { partitiontest.PartitionTest(t) + // create a non-default registry for the metrics in this test + registry := MakeRegistry() + // Test AddMetrics and WriteMetrics with a counter - counter := MakeCounter(MetricName{Name: "gauge-name", Description: "gauge description"}) + counter := MakeCounterUnregistered(MetricName{Name: "gauge-name", Description: "gauge description"}) + counter.Register(registry) + counter.AddUint64(12, nil) - labelCounter := MakeCounter(MetricName{Name: "label-counter", Description: "counter with labels"}) + labelCounter := MakeCounterUnregistered(MetricName{Name: "label-counter", Description: "counter with labels"}) + labelCounter.Register(registry) + labelCounter.AddUint64(5, map[string]string{"label": "a label value"}) results := make(map[string]float64) - DefaultRegistry().AddMetrics(results) + registry.AddMetrics(results) require.Equal(t, 2, len(results), "results", results) require.Contains(t, results, "gauge-name") @@ -44,19 +51,19 @@ func TestWriteAdd(t *testing.T) { require.InDelta(t, 5, results["label-counter_label__a_label_value_"], 0.01) bufBefore := strings.Builder{} - DefaultRegistry().WriteMetrics(&bufBefore, "label") + registry.WriteMetrics(&bufBefore, "label") require.True(t, bufBefore.Len() > 0) - DefaultRegistry().AddMetrics(results) + registry.AddMetrics(results) require.Contains(t, results, "gauge-name") require.InDelta(t, 12, results["gauge-name"], 0.01) // not included in string builder bufAfter := strings.Builder{} - DefaultRegistry().WriteMetrics(&bufAfter, "label") + registry.WriteMetrics(&bufAfter, "label") require.Equal(t, bufBefore.String(), bufAfter.String()) - counter.Deregister(nil) - labelCounter.Deregister(nil) + counter.Deregister(registry) + labelCounter.Deregister(registry) } diff --git a/util/metrics/reporter.go b/util/metrics/reporter.go index 07401414c6..65423691e0 100644 --- a/util/metrics/reporter.go +++ b/util/metrics/reporter.go @@ -121,7 +121,11 @@ func (reporter *MetricReporter) waitForTimeStamp(ctx context.Context) bool { func (reporter *MetricReporter) gatherMetrics() { var buf strings.Builder - DefaultRegistry().WriteMetrics(&buf, reporter.formattedLabels) + if reporter.serviceConfig.registry != nil { + reporter.serviceConfig.registry.WriteMetrics(&buf, reporter.formattedLabels) + } else { + DefaultRegistry().WriteMetrics(&buf, reporter.formattedLabels) + } reporter.lastMetricsBuffer = buf } diff --git a/util/metrics/serviceCommon.go b/util/metrics/serviceCommon.go index 48cf1a146c..fb7e800599 100644 --- a/util/metrics/serviceCommon.go +++ b/util/metrics/serviceCommon.go @@ -30,6 +30,7 @@ type ServiceConfig struct { NodeExporterListenAddress string Labels map[string]string NodeExporterPath string + registry *Registry // if nil, will use DefaultRegistry(). Used for testing } // MetricService represent a single running metric server instance