Skip to content

Commit

Permalink
fix: revert back to recognize only "sampleRate" or "SampleRate" attri…
Browse files Browse the repository at this point in the history
…butes (#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
  • Loading branch information
robbkidd authored Nov 14, 2024
1 parent e7aea2a commit 98048ba
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 11 deletions.
23 changes: 19 additions & 4 deletions otlp/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 ""
}
16 changes: 9 additions & 7 deletions otlp/common_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 98048ba

Please sign in to comment.