Fix unit warnings (#240)

internal/driver/driver.go

@@ -290,7 +290,7 @@ func identifyNumLabelUnits(p *profile.Profile, ui plugin.UI) map[string]string {
 	// Print errors for tags with multiple units associated with
 	// a single key.
 	for k, units := range ignoredUnits {
-		ui.PrintErr(fmt.Sprintf("For tag %s used unit %s, also encountered unit(s) %s", k, numLabelUnits[k], strings.Join(units, ",")))
+		ui.PrintErr(fmt.Sprintf("For tag %s used unit %s, also encountered unit(s) %s", k, numLabelUnits[k], strings.Join(units, ", ")))
 	}
 	return numLabelUnits
 }

internal/driver/driver_test.go

@@ -22,6 +22,7 @@ import (
 	"net"
 	_ "net/http/pprof"
 	"os"
+	"reflect"
 	"regexp"
 	"runtime"
 	"strconv"
@@ -1077,29 +1078,85 @@ func TestIdentifyNumLabelUnits(t *testing.T) {
 		wantIgnoreErrCount int
 	}{
 		{
-			"Multiple keys, different units",
-			[]map[string][]int64{{"key1": {131072}, "key2": {128}}},
-			[]map[string][]string{{"key1": {"bytes"}, "key2": {"kilobytes"}}},
-			map[string]string{"key1": "bytes", "key2": "kilobytes"},
+			"Multiple keys, no units for all keys",
+			[]map[string][]int64{{"keyA": {131072}, "keyB": {128}}},
+			[]map[string][]string{{"keyA": {}, "keyB": {""}}},
+			map[string]string{"keyA": "keyA", "keyB": "keyB"},
 			"",
 			0,
 		},
 		{
-			"One key with different units in same sample",
-			[]map[string][]int64{{"key1": {8, 8}}},
-			[]map[string][]string{{"key1": {"bytes", "kilobytes"}}},
-			map[string]string{"key1": "bytes"},
-			`(For tag key1 used unit bytes, also encountered unit\(s\) kilobytes)`,
+			"Multiple keys, different units for each key",
+			[]map[string][]int64{{"keyA": {131072}, "keyB": {128}}},
+			[]map[string][]string{{"keyA": {"bytes"}, "keyB": {"kilobytes"}}},
+			map[string]string{"keyA": "bytes", "keyB": "kilobytes"},
+			"",
+			0,
+		},
+		{
+			"Multiple keys with multiple values, different units for each key",
+			[]map[string][]int64{{"keyC": {131072, 1}, "keyD": {128, 252}}},
+			[]map[string][]string{{"keyC": {"bytes", "bytes"}, "keyD": {"kilobytes", "kilobytes"}}},
+			map[string]string{"keyC": "bytes", "keyD": "kilobytes"},
+			"",
+			0,
+		},
+		{
+			"Multiple keys with multiple values, some units missing",
+			[]map[string][]int64{{"key1": {131072, 1}, "A": {128, 252}, "key3": {128}, "key4": {1}}, {"key3": {128}, "key4": {1}}},
+			[]map[string][]string{{"key1": {"", "bytes"}, "A": {"kilobytes", ""}, "key3": {""}, "key4": {"hour"}}, {"key3": {"seconds"}, "key4": {""}}},
+			map[string]string{"key1": "bytes", "A": "kilobytes", "key3": "seconds", "key4": "hour"},
+			"",
+			0,
+		},
+		{
+			"One key with three units in same sample",
+			[]map[string][]int64{{"key": {8, 8, 16}}},
+			[]map[string][]string{{"key": {"bytes", "megabytes", "kilobytes"}}},
+			map[string]string{"key": "bytes"},
+			`(For tag key used unit bytes, also encountered unit\(s\) kilobytes, megabytes)`,
+			1,
+		},
+		{
+			"One key with four units in same sample",
+			[]map[string][]int64{{"key": {8, 8, 16, 32}}},
+			[]map[string][]string{{"key": {"bytes", "kilobytes", "a", "megabytes"}}},
+			map[string]string{"key": "bytes"},
+			`(For tag key used unit bytes, also encountered unit\(s\) a, kilobytes, megabytes)`,
+			1,
+		},
+		{
+			"One key with two units in same sample",
+			[]map[string][]int64{{"key": {8, 8}}},
+			[]map[string][]string{{"key": {"bytes", "seconds"}}},
+			map[string]string{"key": "bytes"},
+			`(For tag key used unit bytes, also encountered unit\(s\) seconds)`,
 			1,
 		},
 		{
 			"One key with different units in different samples",
-			[]map[string][]int64{{"key1": {8}}, {"key1": {8}}},
-			[]map[string][]string{{"key1": {"bytes"}}, {"key1": {"kilobytes"}}},
+			[]map[string][]int64{{"key1": {8}}, {"key1": {8}}, {"key1": {8}}},
+			[]map[string][]string{{"key1": {"bytes"}}, {"key1": {"kilobytes"}}, {"key1": {"megabytes"}}},
 			map[string]string{"key1": "bytes"},
-			`(For tag key1 used unit bytes, also encountered unit\(s\) kilobytes)`,
+			`(For tag key1 used unit bytes, also encountered unit\(s\) kilobytes, megabytes)`,
 			1,
 		},
+		{
+			"Key alignment, unit not specified",
+			[]map[string][]int64{{"alignment": {8}}},
+			[]map[string][]string{nil},
+			map[string]string{"alignment": "bytes"},
+			"",
+			0,
+		},
+		{
+			"Key request, unit not specified",
+			[]map[string][]int64{{"request": {8}}, {"request": {8, 8}}},
+			[]map[string][]string{nil, nil},
+			map[string]string{"request": "bytes"},
+			"",
+			0,
+		},
 		{
 			"Check units not over-written for keys with default units",
 			[]map[string][]int64{{
@@ -1133,11 +1190,8 @@ func TestIdentifyNumLabelUnits(t *testing.T) {
 			}
 			testUI := &proftest.TestUI{T: t, AllowRx: test.allowedRx}
 			units := identifyNumLabelUnits(&p, testUI)
-			for key, wantUnit := range test.wantUnits {
-				unit := units[key]
-				if wantUnit != unit {
-					t.Errorf("for key %s, got unit %s, want unit %s", key, unit, wantUnit)
-				}
+			if !reflect.DeepEqual(test.wantUnits, units) {
+				t.Errorf("got %v units, want %v", units, test.wantUnits)
 			}
 			if got, want := testUI.NumAllowRxMatches, test.wantIgnoreErrCount; want != got {
 				t.Errorf("got %d errors logged, want %d errors logged", got, want)

profile/profile.go

@@ -450,20 +450,16 @@ func (p *Profile) Aggregate(inlineFrame, function, filename, linenumber, address
 func (p *Profile) NumLabelUnits() (map[string]string, map[string][]string) {
 	numLabelUnits := map[string]string{}
 	ignoredUnits := map[string]map[string]bool{}
+	encounteredKeys := map[string]bool{}
 
 	// Determine units based on numeric tags for each sample.
 	for _, s := range p.Sample {
-		for k, vs := range s.NumLabel {
-			units := s.NumUnit[k]
-			if len(units) != len(vs) {
-				if _, ok := numLabelUnits[k]; !ok {
-					numLabelUnits[k] = ""
-				} else {
-					ignoredUnits[k] = map[string]bool{"": true}
+		for k := range s.NumLabel {
+			encounteredKeys[k] = true
+			for _, unit := range s.NumUnit[k] {
+				if unit == "" {
+					continue
 				}
-				continue
-			}
-			for _, unit := range units {
 				if wantUnit, ok := numLabelUnits[k]; !ok {
 					numLabelUnits[k] = unit
 				} else if wantUnit != unit {
@@ -476,10 +472,10 @@ func (p *Profile) NumLabelUnits() (map[string]string, map[string][]string) {
 			}
 		}
 	}
-
 	// Infer units for keys without any units associated with
 	// numeric tag values.
-	for key, unit := range numLabelUnits {
+	for key := range encounteredKeys {
+		unit := numLabelUnits[key]
 		if unit == "" {
 			switch key {
 			case "alignment", "request":

profile/profile_test.go

@@ -811,9 +811,9 @@ func locationHash(s *Sample) string {
 	return tb
 }
 
-func TestInferUnits(t *testing.T) {
+func TestNumLabelUnits(t *testing.T) {
 	var tagFilterTests = []struct {
-		name             string
+		desc             string
 		tagVals          []map[string][]int64
 		tagUnits         []map[string][]string
 		wantUnits        map[string]string
@@ -833,6 +833,13 @@ func TestInferUnits(t *testing.T) {
 			map[string]string{"key1": "bytes"},
 			map[string][]string{},
 		},
+		{
+			"One sample, one key with one value, empty unit specified",
+			[]map[string][]int64{{"key1": {8}}},
+			[]map[string][]string{{"key1": {""}}},
+			map[string]string{"key1": "key1"},
+			map[string][]string{},
+		},
 		{
 			"Key bytes, unit not specified",
 			[]map[string][]int64{{"bytes": {8}}},
@@ -862,12 +869,19 @@ func TestInferUnits(t *testing.T) {
 			map[string][]string{},
 		},
 		{
-			"One sample, one key with multiple values and different units",
+			"One sample, one key with multiple values and two different units",
 			[]map[string][]int64{{"key1": {8, 8}}},
 			[]map[string][]string{{"key1": {"bytes", "kilobytes"}}},
 			map[string]string{"key1": "bytes"},
 			map[string][]string{"key1": {"kilobytes"}},
 		},
+		{
+			"One sample, one key with multiple values and three different units",
+			[]map[string][]int64{{"key1": {8, 8}}},
+			[]map[string][]string{{"key1": {"bytes", "megabytes", "kilobytes"}}},
+			map[string]string{"key1": "bytes"},
+			map[string][]string{"key1": {"kilobytes", "megabytes"}},
+		},
 		{
 			"Two samples, one key, different units specified",
 			[]map[string][]int64{{"key1": {8}}, {"key1": {8}}},
@@ -905,24 +919,11 @@ func TestInferUnits(t *testing.T) {
 			p.Sample[i] = &s
 		}
 		units, ignoredUnits := p.NumLabelUnits()
-		for key, wantUnit := range test.wantUnits {
-			unit := units[key]
-			if wantUnit != unit {
-				t.Errorf("%s: for key %s, got unit %s, want unit %s", test.name, key, unit, wantUnit)
-			}
+		if !reflect.DeepEqual(test.wantUnits, units) {
+			t.Errorf("%s: got %v units, want %v", test.desc, units, test.wantUnits)
 		}
-		for key, ignored := range ignoredUnits {
-			wantIgnored := test.wantIgnoredUnits[key]
-			if len(wantIgnored) != len(ignored) {
-				t.Errorf("%s: for key %s, got ignored units %v, got ignored units %v", test.name, key, ignored, wantIgnored)
-				continue
-			}
-			for i, want := range wantIgnored {
-				if got := ignored[i]; want != got {
-					t.Errorf("%s: for key %s, got ignored units %v, want ignored units %v", test.name, key, ignored, wantIgnored)
-					break
-				}
-			}
+		if !reflect.DeepEqual(test.wantIgnoredUnits, ignoredUnits) {
+			t.Errorf("%s: got %v ignored units, want %v", test.desc, ignoredUnits, test.wantIgnoredUnits)
 		}
 	}
 }