From 106e00b8d3bd1882b53ecc3f1dfc9045d30d368d Mon Sep 17 00:00:00 2001
From: jiwen624 <jiwen624@gmail.com>
Date: Mon, 20 Apr 2020 15:28:24 -0700
Subject: [PATCH] metrics count should be positive

---
 v1/ao/internal/metrics/metrics.go        | 26 +++++++++++++++++++++++-
 v1/ao/internal/reporter/reporter_test.go | 12 +++++++++++
 v1/ao/metrics.go                         | 14 ++++---------
 3 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/v1/ao/internal/metrics/metrics.go b/v1/ao/internal/metrics/metrics.go
index 20fc34bf..cdb374a8 100644
--- a/v1/ao/internal/metrics/metrics.go
+++ b/v1/ao/internal/metrics/metrics.go
@@ -20,13 +20,15 @@ import (
 	"github.com/pkg/errors"
 )
 
-// Linux distributions and their identifying files
 const (
 	metricsTransactionsMaxDefault = 200 // default max amount of transaction names we allow per cycle
 	metricsHistPrecisionDefault   = 2   // default histogram precision
 
 	metricsTagNameLengthMax  = 64  // max number of characters for tag names
 	metricsTagValueLengthMax = 255 // max number of characters for tag values
+
+	// MaxTagsCount is the maximum number of tags allowed
+	MaxTagsCount = 50
 )
 
 // Special transaction names
@@ -59,6 +61,10 @@ const (
 var (
 	// ErrExceedsMetricsCountLimit indicates there are too many distinct metrics.
 	ErrExceedsMetricsCountLimit = errors.New("exceeds metrics count limit per flush interval")
+	// ErrExceedsTagsCountLimit indicates there are too many tags
+	ErrExceedsTagsCountLimit = errors.New("exceeds tags count limit")
+	// ErrMetricsWithNonPositiveCount indicates the count is negative or zero
+	ErrMetricsWithNonPositiveCount = errors.New("metrics with non-positive count")
 )
 
 // SpanMessage defines a span message
@@ -409,11 +415,17 @@ func (m *Measurements) Clone() *Measurements {
 
 // Summary submits the summary measurement to the reporter.
 func (m *Measurements) Summary(name string, value float64, opts MetricOptions) error {
+	if err := opts.validate(); err != nil {
+		return err
+	}
 	return m.recordWithSoloTags(name, opts.Tags, value, opts.Count, true)
 }
 
 // Increment submits the incremental measurement to the reporter.
 func (m *Measurements) Increment(name string, opts MetricOptions) error {
+	if err := opts.validate(); err != nil {
+		return err
+	}
 	return m.recordWithSoloTags(name, opts.Tags, 0, opts.Count, false)
 }
 
@@ -424,6 +436,18 @@ type MetricOptions struct {
 	Tags    map[string]string
 }
 
+func (mo *MetricOptions) validate() error {
+	if len(mo.Tags) > MaxTagsCount {
+		return ErrExceedsTagsCountLimit
+	}
+
+	if mo.Count <= 0 {
+		return ErrMetricsWithNonPositiveCount
+	}
+
+	return nil
+}
+
 func addRuntimeMetrics(bbuf *bson.Buffer, index *int) {
 	// category runtime
 	addMetricsValue(bbuf, index, "trace.go.runtime.NumGoroutine", runtime.NumGoroutine())
diff --git a/v1/ao/internal/reporter/reporter_test.go b/v1/ao/internal/reporter/reporter_test.go
index fd34e68e..0b6736dd 100644
--- a/v1/ao/internal/reporter/reporter_test.go
+++ b/v1/ao/internal/reporter/reporter_test.go
@@ -595,6 +595,18 @@ func TestCustomMetrics(t *testing.T) {
 		customMetrics: metrics.NewMeasurements(true, grpcMetricIntervalDefault, 500),
 	}
 
+	// Test non-positive count
+	assert.NotNil(t, r.CustomSummaryMetric("Summary", 1.1, metrics.MetricOptions{
+		Count:   0,
+		HostTag: true,
+		Tags:    map[string]string{"hello": "world"},
+	}))
+	assert.NotNil(t, r.CustomIncrementMetric("Incremental", metrics.MetricOptions{
+		Count:   -1,
+		HostTag: true,
+		Tags:    map[string]string{"hi": "globe"},
+	}))
+
 	r.CustomSummaryMetric("Summary", 1.1, metrics.MetricOptions{
 		Count:   1,
 		HostTag: true,
diff --git a/v1/ao/metrics.go b/v1/ao/metrics.go
index d029b317..b4395405 100644
--- a/v1/ao/metrics.go
+++ b/v1/ao/metrics.go
@@ -3,8 +3,6 @@
 package ao
 
 import (
-	"errors"
-
 	"github.com/appoptics/appoptics-apm-go/v1/ao/internal/metrics"
 	"github.com/appoptics/appoptics-apm-go/v1/ao/internal/reporter"
 )
@@ -14,31 +12,27 @@ type MetricOptions = metrics.MetricOptions
 
 const (
 	// MaxTagsCount is the maximum number of tags allowed.
-	MaxTagsCount = 50
+	MaxTagsCount = metrics.MaxTagsCount
 )
 
 // The measurements submission errors
 var (
 	// ErrExceedsTagsCountLimit indicates the count of tags exceeds the limit
-	ErrExceedsTagsCountLimit = errors.New("exceeds tags count limit")
+	ErrExceedsTagsCountLimit = metrics.ErrExceedsTagsCountLimit
 	// ErrExceedsMetricsCountLimit indicates there are too many distinct measurements in a flush cycle.
 	ErrExceedsMetricsCountLimit = metrics.ErrExceedsMetricsCountLimit
+	// ErrMetricsWithNonPositiveCount indicates the count is negative or zero
+	ErrMetricsWithNonPositiveCount = metrics.ErrMetricsWithNonPositiveCount
 )
 
 // SummaryMetric submits a summary type measurement to the reporter. The measurements
 // will be collected in the background and reported periodically.
 func SummaryMetric(name string, value float64, opts MetricOptions) error {
-	if len(opts.Tags) > MaxTagsCount {
-		return ErrExceedsTagsCountLimit
-	}
 	return reporter.SummaryMetric(name, value, opts)
 }
 
 // IncrementMetric submits a incremental measurement to the reporter. The measurements
 // will be collected in the background and reported periodically.
 func IncrementMetric(name string, opts MetricOptions) error {
-	if len(opts.Tags) > MaxTagsCount {
-		return ErrExceedsTagsCountLimit
-	}
 	return reporter.IncrementMetric(name, opts)
 }