From 98048bac61dc8b45bce70a38ea807f53708dcf92 Mon Sep 17 00:00:00 2001
From: Robb Kidd <robbkidd@honeycomb.io>
Date: Thu, 14 Nov 2024 16:12:18 -0500
Subject: [PATCH] fix: revert back to recognize only "sampleRate" or
 "SampleRate" attributes (#284)

> What kinds of sample rates do you usually have here?
> We got both kinds: `sampleRate` and `SampleRate`.

## Which problem is this PR solving?

1. A predicted performance regression during ingest: iterating over all
attributes looking for a matching key name is much slower than 2 lookups
into the attributes map.
2. Non-deterministic behavior in the probably-rare case of attributes
containing multiple "samplerate" keys of differing capitalization, which
would be astonishingly difficult to troubleshoot after ingest

## Short description of the changes

- update the test to assert the previous behavior of recognizing only
`sampleRate` and `SampleRate`
- revert getSampleRateKey function back to two-key lookup behavior
- leave notes to Future Us
---
 otlp/common.go      | 23 +++++++++++++++++++----
 otlp/common_test.go | 16 +++++++++-------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/otlp/common.go b/otlp/common.go
index 3ce6638..0f53535 100644
--- a/otlp/common.go
+++ b/otlp/common.go
@@ -639,11 +639,26 @@ func getSampleRate(attrs map[string]interface{}) int32 {
 	return sampleRate
 }
 
+// getSampleRateKey determines if a map of attributes includes
+// one of the two recognized variations of the sample rate key name:
+//   - sampleRate (preferred)
+//   - SampleRate
+//
+// Returns the key name if found, or an empty string if not.
 func getSampleRateKey(attrs map[string]interface{}) string {
-	for key := range attrs {
-		if strings.EqualFold(key, "sampleRate") {
-			return key
-		}
+	// Why only two keys?
+	//  1. 1-2 map lookups is faster than looping over the keys.
+	//  2. An explicit order of precedence.
+	//
+	// Limiting to two variations with an order of precedence
+	// dramatically simplifies troubleshooting unexpected sample
+	// rate behavior if somehow data were to have multiple sampleRate
+	// keys with different capitalization variations.
+	if _, ok := attrs["sampleRate"]; ok {
+		return "sampleRate"
+	}
+	if _, ok := attrs["SampleRate"]; ok {
+		return "SampleRate"
 	}
 	return ""
 }
diff --git a/otlp/common_test.go b/otlp/common_test.go
index 872809b..113877f 100644
--- a/otlp/common_test.go
+++ b/otlp/common_test.go
@@ -787,18 +787,20 @@ func TestNoSampleRateKeyReturnOne(t *testing.T) {
 	assert.Equal(t, int32(1), sampleRate)
 }
 
-func TestCanDetectSampleRateCapitalizations(t *testing.T) {
+func TestSampleRateKeyVariations(t *testing.T) {
 	tests := []struct {
 		name  string
 		attrs map[string]interface{}
 		want  string
 	}{
-		{"lowercase", map[string]interface{}{"samplerate": 10}, "samplerate"},
-		{"UPPERCASE", map[string]interface{}{"SAMPLERATE": 10}, "SAMPLERATE"},
-		{"camelCase", map[string]interface{}{"sampleRate": 10}, "sampleRate"},
-		{"PascalCase", map[string]interface{}{"SampleRate": 10}, "SampleRate"},
-		{"MiXeDcAsE", map[string]interface{}{"SaMpLeRaTe": 10}, "SaMpLeRaTe"},
-		{"bad", map[string]interface{}{"sample_rate": 10}, ""},
+		// ACCEPTED - only accept the two variations we've done in the past
+		{"ACCEPTED/camelCase", map[string]interface{}{"sampleRate": 10}, "sampleRate"},
+		{"ACCEPTED/PascalCase", map[string]interface{}{"SampleRate": 10}, "SampleRate"},
+		// IGNORED - other variations will be ignored
+		{"INGORED/lowercase", map[string]interface{}{"samplerate": 10}, ""},
+		{"INGORED/UPPERCASE", map[string]interface{}{"SAMPLERATE": 10}, ""},
+		{"INGORED/MiXeDcAsE", map[string]interface{}{"SaMpLeRaTe": 10}, ""},
+		{"INGORED/snake_case", map[string]interface{}{"sample_rate": 10}, ""},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {