From a5fc9af5918382fc12bd3379e651c84a15fc0596 Mon Sep 17 00:00:00 2001
From: Shankar Nair <shankar.nair@uber.com>
Date: Tue, 3 Oct 2023 16:06:33 -0700
Subject: [PATCH] review comments

---
 m3/config.go        |  2 +-
 m3/reporter.go      |  4 ++--
 m3/reporter_test.go | 22 ++++++++++++++--------
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/m3/config.go b/m3/config.go
index e8861e0d..ccfb51ce 100644
--- a/m3/config.go
+++ b/m3/config.go
@@ -70,6 +70,6 @@ func (c Configuration) NewReporter() (Reporter, error) {
 		MaxPacketSizeBytes:          c.PacketSize,
 		IncludeHost:                 c.IncludeHost,
 		HistogramBucketTagPrecision: c.HistogramBucketTagPrecision,
-		CommonTagsInternal:          c.CommonTagsInternal,
+		InternalTags:                c.CommonTagsInternal,
 	})
 }
diff --git a/m3/reporter.go b/m3/reporter.go
index 8c3b1e3d..c930d5c2 100644
--- a/m3/reporter.go
+++ b/m3/reporter.go
@@ -149,7 +149,7 @@ type Options struct {
 	HistogramBucketIDName       string
 	HistogramBucketName         string
 	HistogramBucketTagPrecision uint
-	CommonTagsInternal          map[string]string
+	InternalTags                map[string]string
 }
 
 // NewReporter creates a new M3 reporter.
@@ -290,7 +290,7 @@ func NewReporter(opts Options) (Reporter, error) {
 		"version": tally.Version,
 	}
 
-	for k, v := range opts.CommonTagsInternal {
+	for k, v := range opts.InternalTags {
 		internalTags[k] = v
 	}
 
diff --git a/m3/reporter_test.go b/m3/reporter_test.go
index 61a36bb4..6e5bbb0a 100644
--- a/m3/reporter_test.go
+++ b/m3/reporter_test.go
@@ -567,7 +567,7 @@ func TestReporterCommmonTagsInternal(t *testing.T) {
 	go server.Serve()
 	defer server.Close()
 
-	commonTagsInternal := map[string]string{
+	internalTags := map[string]string{
 		"internal1": "test1",
 		"internal2": "test2",
 	}
@@ -577,8 +577,9 @@ func TestReporterCommmonTagsInternal(t *testing.T) {
 		Service:            "test-service",
 		CommonTags:         defaultCommonTags,
 		MaxQueueSize:       queueSize,
+		IncludeHost:        true,
 		MaxPacketSizeBytes: maxPacketSize,
-		CommonTagsInternal: commonTagsInternal,
+		InternalTags:       internalTags,
 	})
 	require.NoError(t, err)
 	defer r.Close()
@@ -595,7 +596,7 @@ func TestReporterCommmonTagsInternal(t *testing.T) {
 	for _, metric := range metrics {
 		if strings.HasPrefix(metric.Name, "tally.internal") {
 			numInternalMetricsActual++
-			for k, v := range commonTagsInternal {
+			for k, v := range internalTags {
 				require.True(t, tagEquals(metric.Tags, k, v))
 			}
 		} else {
@@ -603,6 +604,11 @@ func TestReporterCommmonTagsInternal(t *testing.T) {
 			require.False(t, tagIncluded(metric.Tags, "internal1"))
 			require.False(t, tagIncluded(metric.Tags, "internal2"))
 		}
+		// The following tags should not be present as part of the individual metrics
+		// as they are common tags.
+		require.False(t, tagIncluded(metric.Tags, "host"))
+		require.False(t, tagIncluded(metric.Tags, "instance"))
+		require.False(t, tagIncluded(metric.Tags, "service"))
 	}
 	require.Equal(t, internalMetrics, numInternalMetricsActual)
 }
@@ -636,12 +642,12 @@ type fakeM3ServerPackets struct {
 }
 
 // newFakeM3Server creates a new fake M3 server that listens on a random port
-// and returns the address.
+// and returns the server.
 // The server will wait for the given wait group to be done before returning.
-// If countBatches is true, the server will wait for a batch to be received
-// before returning.
-// But if countBatches is false, the server will wait for a metric to be received
-// before returning.
+// If countBatches is true, the server will wait consider the wg.Add()s to be
+// representing batches and will do a eg.Done() for each encountered batch.
+// But if countBatches is false, the server will do the same thing but for individual
+// metrics instead of batches.
 func newFakeM3Server(t *testing.T, wg *sync.WaitGroup, countBatches bool, protocol Protocol) *fakeM3Server {
 	service := newFakeM3Service(wg, countBatches)
 	processor := m3thrift.NewM3Processor(service)