From 451d00ecdad71b8a4f100bbd663bbbffbd510387 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Thu, 25 Jan 2024 08:53:45 -0500 Subject: [PATCH] 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 Co-authored-by: Faith Chikwekwe --- config/config_test.go | 32 ++++++++++++++++++++-- config/file_config.go | 64 +++++++++++++++++++++++++++++++++++-------- logger/honeycomb.go | 2 +- rules.json | 1 + 4 files changed, 84 insertions(+), 15 deletions(-) create mode 100644 rules.json diff --git a/config/config_test.go b/config/config_test.go index e5788454e3..577778e085 100644 --- 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 index a771ae05a5..1ad38f21fc 100644 --- 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 index 8dfd15bd20..829fd04ef8 100644 --- 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 new file mode 100644 index 0000000000..acdcb9c29a --- /dev/null +++ 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}}}}