fix: allow config bools to default to true (#969)

## Which problem is this PR solving? - #954 ## Short description of the changes - add a failing test to reproduce the problem - updated config struct fields that are bools defaulting to true to be bool pointers so that nil represents the unset state - updated getter functions to handle the pointers - [x] add a GetSamplerEnabled() function to HoneycombLoggerConfig to handle pointer defreferencing ## Maybe TODO - [ ] use something (reflect?) to do struct tag lookups to find default values in the getter functions instead of declaring defaults in multiple places - [ ] keep the default-true-bool-overridden-with-false test? - [ ] contribute documentation about bool pointer workaround to upstream defaults library? --------- Co-authored-by: Faith Chikwekwe <[email protected]> Co-authored-by: Faith Chikwekwe <[email protected]>
honeycombio · Jan 25, 2024 · 451d00e · 451d00e
1 parent 63e6e81
commit 451d00e
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 15 deletions.
diff --git a/config/config_test.go b/config/config_test.go
@@ -616,7 +616,7 @@ func TestHoneycombLoggerConfig(t *testing.T) {
 	assert.Equal(t, "http://honeycomb.io", loggerConfig.APIHost)
 	assert.Equal(t, "1234", loggerConfig.APIKey)
 	assert.Equal(t, "loggerDataset", loggerConfig.Dataset)
-	assert.Equal(t, true, loggerConfig.SamplerEnabled)
+	assert.Equal(t, true, loggerConfig.GetSamplerEnabled())
 	assert.Equal(t, 5, loggerConfig.SamplerThroughput)
 }
 
@@ -639,7 +639,7 @@ func TestHoneycombLoggerConfigDefaults(t *testing.T) {
 
 	assert.NoError(t, err)
 
-	assert.Equal(t, true, loggerConfig.SamplerEnabled)
+	assert.Equal(t, true, loggerConfig.GetSamplerEnabled())
 	assert.Equal(t, 10, loggerConfig.SamplerThroughput)
 }
 
@@ -663,7 +663,7 @@ func TestHoneycombGRPCConfigDefaults(t *testing.T) {
 	assert.Equal(t, "localhost:4343", a)
 
 	grpcConfig := c.GetGRPCConfig()
-	assert.Equal(t, true, grpcConfig.Enabled)
+	assert.Equal(t, true, *grpcConfig.Enabled)
 	assert.Equal(t, "localhost:4343", grpcConfig.ListenAddr)
 	assert.Equal(t, 1*time.Minute, time.Duration(grpcConfig.MaxConnectionIdle))
 	assert.Equal(t, 3*time.Minute, time.Duration(grpcConfig.MaxConnectionAge))
@@ -890,6 +890,32 @@ func TestHoneycombIdFieldsConfigDefault(t *testing.T) {
 	assert.Equal(t, []string{"trace.parent_id", "parentId"}, c.GetParentIdFieldNames())
 }
 
+func TestOverrideConfigDefaults(t *testing.T) {
+	/// Check that fields that default to true can be set to false
+	cm := makeYAML(
+		"General.ConfigurationVersion", 2,
+		"RefineryTelemetry.AddSpanCountToRoot", false,
+		"RefineryTelemetry.AddHostMetadataToTrace", false,
+		"HoneycombLogger.SamplerEnabled", false,
+		"Specialized.CompressPeerCommunication", false,
+		"GRPCServerParameters.Enabled", false,
+	)
+	rm := makeYAML("ConfigVersion", 2)
+	config, rules := createTempConfigs(t, cm, rm)
+	defer os.Remove(rules)
+	defer os.Remove(config)
+	c, err := getConfig([]string{"--no-validate", "--config", config, "--rules_config", rules})
+	assert.NoError(t, err)
+
+	assert.Equal(t, false, c.GetAddSpanCountToRoot())
+	assert.Equal(t, false, c.GetAddHostMetadataToTrace())
+	loggerConfig, err := c.GetHoneycombLoggerConfig()
+	assert.NoError(t, err)
+	assert.Equal(t, false, loggerConfig.GetSamplerEnabled())
+	assert.Equal(t, false, c.GetCompressPeerCommunication())
+	assert.Equal(t, false, c.GetGRPCEnabled())
+}
+
 func TestMemorySizeUnmarshal(t *testing.T) {
 	tests := []struct {
 		name     string

diff --git a/config/file_config.go b/config/file_config.go
@@ -89,10 +89,10 @@ type AccessKeyConfig struct {
 }
 
 type RefineryTelemetryConfig struct {
-	AddRuleReasonToTrace   bool `yaml:"AddRuleReasonToTrace"`
-	AddSpanCountToRoot     bool `yaml:"AddSpanCountToRoot" default:"true"`
-	AddCountsToRoot        bool `yaml:"AddCountsToRoot"`
-	AddHostMetadataToTrace bool `yaml:"AddHostMetadataToTrace" default:"true"`
+	AddRuleReasonToTrace   bool  `yaml:"AddRuleReasonToTrace"`
+	AddSpanCountToRoot     *bool `yaml:"AddSpanCountToRoot" default:"true"` // Avoid pointer woe on access, use GetAddSpanCountToRoot() instead.
+	AddCountsToRoot        bool  `yaml:"AddCountsToRoot"`
+	AddHostMetadataToTrace *bool `yaml:"AddHostMetadataToTrace" default:"true"` // Avoid pointer woe on access, use GetAddHostMetadataToTrace() instead.
 }
 
 type TracesConfig struct {
@@ -119,10 +119,21 @@ type HoneycombLoggerConfig struct {
 	APIHost           string `yaml:"APIHost" default:"https://api.honeycomb.io"`
 	APIKey            string `yaml:"APIKey" cmdenv:"HoneycombLoggerAPIKey,HoneycombAPIKey"`
 	Dataset           string `yaml:"Dataset" default:"Refinery Logs"`
-	SamplerEnabled    bool   `yaml:"SamplerEnabled" default:"true"`
+	SamplerEnabled    *bool  `yaml:"SamplerEnabled" default:"true"` // Avoid pointer woe on access, use GetSamplerEnabled() instead.
 	SamplerThroughput int    `yaml:"SamplerThroughput" default:"10"`
 }
 
+// GetSamplerEnabled returns whether configuration has enabled sampling of
+// Refinery's own logs destined for Honeycomb.
+func (c *HoneycombLoggerConfig) GetSamplerEnabled() (enabled bool) {
+	if c.SamplerEnabled == nil {
+		enabled = true
+	} else {
+		enabled = *c.SamplerEnabled
+	}
+	return enabled
+}
+
 type StdoutLoggerConfig struct {
 	Structured        bool `yaml:"Structured" default:"false"`
 	SamplerEnabled    bool `yaml:"SamplerEnabled" `
@@ -217,7 +228,7 @@ type BufferSizeConfig struct {
 
 type SpecializedConfig struct {
 	EnvironmentCacheTTL       Duration          `yaml:"EnvironmentCacheTTL" default:"1h"`
-	CompressPeerCommunication bool              `yaml:"CompressPeerCommunication" default:"true"`
+	CompressPeerCommunication *bool             `yaml:"CompressPeerCommunication" default:"true"` // Avoid pointer woe on access, use GetCompressPeerCommunication() instead.
 	AdditionalAttributes      map[string]string `yaml:"AdditionalAttributes" default:"{}"`
 }
 
@@ -230,7 +241,7 @@ type IDFieldsConfig struct {
 // by refinery's own GRPC server:
 // https://pkg.go.dev/google.golang.org/grpc/keepalive#ServerParameters
 type GRPCServerParameters struct {
-	Enabled               bool       `yaml:"Enabled" default:"true"`
+	Enabled               *bool      `yaml:"Enabled" default:"true"` // Avoid pointer woe on access, use GetGRPCEnabled() instead.
 	ListenAddr            string     `yaml:"ListenAddr" cmdenv:"GRPCListenAddr"`
 	MaxConnectionIdle     Duration   `yaml:"MaxConnectionIdle" default:"1m"`
 	MaxConnectionAge      Duration   `yaml:"MaxConnectionAge" default:"3m"`
@@ -477,13 +488,28 @@ func (f *fileConfig) GetCompressPeerCommunication() bool {
 	f.mux.RLock()
 	defer f.mux.RUnlock()
 
-	return f.mainConfig.Specialized.CompressPeerCommunication
+	var compressPeerCommunication bool
+	if f.mainConfig.Specialized.CompressPeerCommunication == nil {
+		compressPeerCommunication = true
+	} else {
+		compressPeerCommunication = *f.mainConfig.Specialized.CompressPeerCommunication
+	}
+
+	return compressPeerCommunication
 }
 
 func (f *fileConfig) GetGRPCEnabled() bool {
 	f.mux.RLock()
 	defer f.mux.RUnlock()
-	return f.mainConfig.GRPCServerParameters.Enabled
+
+	var enabled bool
+	if f.mainConfig.GRPCServerParameters.Enabled == nil {
+		enabled = true
+	} else {
+		enabled = *f.mainConfig.GRPCServerParameters.Enabled
+	}
+
+	return enabled
 }
 
 func (f *fileConfig) GetGRPCListenAddr() (string, error) {
@@ -785,7 +811,15 @@ func (f *fileConfig) GetAddHostMetadataToTrace() bool {
 	f.mux.RLock()
 	defer f.mux.RUnlock()
 
-	return f.mainConfig.Telemetry.AddHostMetadataToTrace
+	var addHostMetadataToTrace bool
+
+	if f.mainConfig.Telemetry.AddHostMetadataToTrace == nil {
+		addHostMetadataToTrace = true // TODO: the default, but maybe look that up on the struct?
+	} else {
+		addHostMetadataToTrace = *f.mainConfig.Telemetry.AddHostMetadataToTrace
+	}
+
+	return addHostMetadataToTrace
 }
 
 func (f *fileConfig) GetAddRuleReasonToTrace() bool {
@@ -834,7 +868,15 @@ func (f *fileConfig) GetAddSpanCountToRoot() bool {
 	f.mux.RLock()
 	defer f.mux.RUnlock()
 
-	return f.mainConfig.Telemetry.AddSpanCountToRoot
+	var addSpanCountToRoot bool
+
+	if f.mainConfig.Telemetry.AddSpanCountToRoot == nil {
+		addSpanCountToRoot = true // TODO: lookup default from struct
+	} else {
+		addSpanCountToRoot = *f.mainConfig.Telemetry.AddSpanCountToRoot
+	}
+
+	return addSpanCountToRoot
 }
 
 func (f *fileConfig) GetAddCountsToRoot() bool {

diff --git a/logger/honeycomb.go b/logger/honeycomb.go
@@ -59,7 +59,7 @@ func (h *HoneycombLogger) Start() error {
 		}
 	}
 
-	if loggerConfig.SamplerEnabled {
+	if loggerConfig.GetSamplerEnabled() {
 		h.sampler = &dynsampler.PerKeyThroughput{
 			ClearFrequencyDuration: 10 * time.Second,
 			PerKeyThroughputPerSec: loggerConfig.SamplerThroughput,

diff --git a/rules.json b/rules.json
@@ -0,0 +1 @@
+{"RulesVersion":2,"Samplers":{"__default__":{"DeterministicSampler":{"SampleRate":1}},"TheNewWorld":{"EMADynamicSampler":{"GoalSampleRate":6,"AdjustmentInterval":"2s","Weight":0.5,"AgeOutValue":0.5,"BurstMultiple":200,"BurstDetectionDelay":30,"FieldList":["error","url","status"],"UseTraceLength":false,"MaxKeys":2000}}}}
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		{"RulesVersion":2,"Samplers":{"__default__":{"DeterministicSampler":{"SampleRate":1}},"TheNewWorld":{"EMADynamicSampler":{"GoalSampleRate":6,"AdjustmentInterval":"2s","Weight":0.5,"AgeOutValue":0.5,"BurstMultiple":200,"BurstDetectionDelay":30,"FieldList":["error","url","status"],"UseTraceLength":false,"MaxKeys":2000}}}}