Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow config bools to default to true #969

Merged
merged 7 commits into from
Jan 25, 2024
Merged

fix: allow config bools to default to true #969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e1b3ccd
add test to repro bools unable to default to true
robbkidd Jan 18, 2024
107265e
use a bool pointer so defaults lib can detect unset state
robbkidd Jan 18, 2024
b62be6d
convert grpcConfig.Enabled to a bool pointer
robbkidd Jan 18, 2024
c33b520
turn more default-true-bools into pointers
robbkidd Jan 18, 2024
888f668
wrap access to the SamplingEnabled bool pointer in a function
robbkidd Jan 19, 2024
dc44554
more doc comments recommending using the getter functions
robbkidd Jan 22, 2024
35f4f89
test: add comment to override test
fchikwekwe Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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)
}

Expand All @@ -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))
Expand Down Expand Up @@ -890,6 +890,32 @@ func TestHoneycombIdFieldsConfigDefault(t *testing.T) {
assert.Equal(t, []string{"trace.parent_id", "parentId"}, c.GetParentIdFieldNames())
}

func TestOverrideConfigDefaults(t *testing.T) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in the first commit of this PR, demonstrate the 5 bools that default to true don't. Not sure if we keep this test long-term.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just add a comment to this explaining why it exists.

/// 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
Expand Down
64 changes: 53 additions & 11 deletions config/file_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -119,10 +119,21 @@ type HoneycombLoggerConfig struct {
APIHost string `yaml:"APIHost" default:"https://api.honeycomb.io"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in scope for this PR, but accessor functions for all fields on HoneycomLoggerConfig would be nice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I struggle with this; every accessor adds one form of tech debt to save another form of tech debt. In particular, when I broke up the config into subsections, I deliberately did not add accessors for everything.

I don't feel strongly against it, I just wish there was a better way (JS/java properties would be nice but Go doesn't give us those).

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.
Copy link
Member Author

@robbkidd robbkidd Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tossed this comment on the field so that it would be shown by IDEs that do the hover box thing. Like a ⚠️ sign saying "Hey, maybe don't use this directly."

🤔 ... which I suppose is a comment that could be added to all the new *bool fields.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this comment to all the *bools in a separate commit (easily removed from this PR if that's too much commentary).

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" `
Expand Down Expand Up @@ -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:"{}"`
}

Expand All @@ -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"`
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These true values are hardcoded, but we should try and lookup these values from the struct tag. That will require us to use the reflect package mimicking some behavior from the defaults package. Is this something that we want in the scope of this fix?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I'm happy with the hardcoded true values that are only present on struct values that are currently type bool and default true. It might be nice to write a helper function that we can add to add struct values to check is type is bool and default is true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robbkidd suggested that we either add some code to the validate function to check for struct tag type and value. Or perhaps there is a test that we can add later on to validate any changes to struct tags.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of reading the tags, we talked about something like:

type BoolDefaultTrue *bool

That would give us the hook to do some more useful linting or other work to make sure that we keep the contract for those types.

} 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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion logger/honeycomb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions rules.json
View file
Open in desktop
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}}}}