fix: allow config bools to default to true #969

robbkidd · 2024-01-18T20:53:09Z

Which problem is this PR solving?

Refinery overwrites config settings by applying defaults after config was loaded #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
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?

kentquirk · 2024-01-18T20:55:38Z

Do you want to make this a draft PR until you have code for it?

robbkidd · 2024-01-18T21:23:29Z

Do you want to make this a draft PR until you have code for it?

Certainly! Drafted ... and there should be code for it shortly.

UPDATE: Code added. As is my wont, walking our commits might help understanding the whys and whats of the changes.

Co-authored-by: Faith Chikwekwe <[email protected]>

Because bool's zero value is false, defaults library cannot determine if a field has not been set. Use a pointer instead! Zero for an unset pointer is nil. Co-authored-by: Faith Chikwekwe <[email protected]>

Co-authored-by: Faith Chikwekwe <[email protected]>

fchikwekwe · 2024-01-18T21:59:51Z

config/file_config.go

-	return f.mainConfig.Specialized.CompressPeerCommunication
+	var compressPeerCommunication bool
+	if f.mainConfig.Specialized.CompressPeerCommunication == nil {
+		compressPeerCommunication = true


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?

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.

@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.

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.

robbkidd · 2024-01-18T22:00:01Z

config/config_test.go

@@ -890,6 +890,31 @@ func TestHoneycombIdFieldsConfigDefault(t *testing.T) {
 	assert.Equal(t, []string{"trace.parent_id", "parentId"}, c.GetParentIdFieldNames())
 }

+func TestOverrideConfigDefaults(t *testing.T) {


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.

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

fchikwekwe · 2024-01-18T22:01:38Z

logger/honeycomb.go

@@ -59,7 +59,7 @@ func (h *HoneycombLogger) Start() error {
 		}
 	}

-	if loggerConfig.SamplerEnabled {
+	if *loggerConfig.SamplerEnabled {


We should consider adding a getter function for HoneycombLogger.SamplerEnabled so that we do not have to dereference in every time Sampler Enabled is used.

A loggerConfig.GetSamplerEnabled() function would also do away with the shenanigans used in test to get a pointer to a bool.

During a chat this morning, @kentquirk, @fchikwekwe, and I agreed we should make the SamplerEnabled bool-pointer private and add a getter function to smooth the bool interface. That'll be work on this PR with other ideas to reduce tech debt in a follow-up issue.

And I learn that I cannot make the SamplerEnabled field private without breaking the YAML marshal/unmarshal. Still adding the function, though!

fchikwekwe · 2024-01-19T18:49:45Z

config/file_config.go

@@ -119,10 +119,21 @@ type HoneycombLoggerConfig struct {
 	APIHost           string `yaml:"APIHost" default:"https://api.honeycomb.io"`


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

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).

robbkidd · 2024-01-19T18:51:05Z

config/file_config.go

@@ -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.


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.

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

kentquirk

I think this is pretty good, and the only thing I'd like to see changed is to define that type for a bool that defaults to true, and use that appropriately; we don't have to get into modifying the defaults package or validating specially unless you see a really easy solution. I wouldn't spend a lot more time on it.

kentquirk · 2024-01-23T18:22:50Z

config/config_test.go

@@ -890,6 +890,31 @@ func TestHoneycombIdFieldsConfigDefault(t *testing.T) {
 	assert.Equal(t, []string{"trace.parent_id", "parentId"}, c.GetParentIdFieldNames())
 }

+func TestOverrideConfigDefaults(t *testing.T) {


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

kentquirk · 2024-01-23T18:24:58Z

config/file_config.go

@@ -119,10 +119,21 @@ type HoneycombLoggerConfig struct {
 	APIHost           string `yaml:"APIHost" default:"https://api.honeycomb.io"`


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).

kentquirk · 2024-01-23T18:30:08Z

config/file_config.go

-	return f.mainConfig.Specialized.CompressPeerCommunication
+	var compressPeerCommunication bool
+	if f.mainConfig.Specialized.CompressPeerCommunication == nil {
+		compressPeerCommunication = true


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.

fchikwekwe · 2024-01-25T13:47:36Z

Yes, I played around with adding the boolean true type a little bit this week, but I'll bump the rest of this work to be completed in #970.

robbkidd requested a review from a team as a code owner January 18, 2024 20:53

fchikwekwe marked this pull request as draft January 18, 2024 21:22

robbkidd force-pushed the fabb.bools-should-default-to-true-tho branch from 138ed07 to 4a6404a Compare January 18, 2024 21:49

robbkidd and others added 2 commits January 18, 2024 16:53

add test to repro bools unable to default to true

e1b3ccd

Co-authored-by: Faith Chikwekwe <[email protected]>

use a bool pointer so defaults lib can detect unset state

107265e

Because bool's zero value is false, defaults library cannot determine if a field has not been set. Use a pointer instead! Zero for an unset pointer is nil. Co-authored-by: Faith Chikwekwe <[email protected]>

robbkidd force-pushed the fabb.bools-should-default-to-true-tho branch from 4a6404a to e2b420a Compare January 18, 2024 21:54

robbkidd and others added 2 commits January 18, 2024 16:56

convert grpcConfig.Enabled to a bool pointer

b62be6d

Co-authored-by: Faith Chikwekwe <[email protected]>

turn more default-true-bools into pointers

c33b520

Co-authored-by: Faith Chikwekwe <[email protected]>

robbkidd force-pushed the fabb.bools-should-default-to-true-tho branch from e2b420a to c33b520 Compare January 18, 2024 21:56

fchikwekwe reviewed Jan 18, 2024

View reviewed changes

robbkidd commented Jan 18, 2024

View reviewed changes

fchikwekwe reviewed Jan 18, 2024

View reviewed changes

robbkidd assigned robbkidd and fchikwekwe Jan 18, 2024

robbkidd requested a review from a team January 18, 2024 22:19

robbkidd added type: bug Something isn't working version: bump patch A PR with release-worthy changes and is backwards-compatible. labels Jan 18, 2024

robbkidd marked this pull request as ready for review January 18, 2024 22:20

wrap access to the SamplingEnabled bool pointer in a function

888f668

fchikwekwe reviewed Jan 19, 2024

View reviewed changes

robbkidd commented Jan 19, 2024

View reviewed changes

more doc comments recommending using the getter functions

dc44554

fchikwekwe mentioned this pull request Jan 22, 2024

Refinery overwrites config settings by applying defaults after config was loaded #954

Closed

kentquirk approved these changes Jan 23, 2024

View reviewed changes

test: add comment to override test

35f4f89

fchikwekwe merged commit 451d00e into main Jan 25, 2024
3 checks passed

fchikwekwe deleted the fabb.bools-should-default-to-true-tho branch January 25, 2024 13:53

fchikwekwe mentioned this pull request Jan 26, 2024

Config bools that default to true require more work #970

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow config bools to default to true #969

fix: allow config bools to default to true #969

robbkidd commented Jan 18, 2024 •

edited by fchikwekwe

Loading

kentquirk commented Jan 18, 2024

robbkidd commented Jan 18, 2024 •

edited

Loading

fchikwekwe Jan 18, 2024

fchikwekwe Jan 18, 2024

fchikwekwe Jan 18, 2024

kentquirk Jan 23, 2024

robbkidd Jan 18, 2024

kentquirk Jan 23, 2024

fchikwekwe Jan 18, 2024

robbkidd Jan 18, 2024 •

edited

Loading

robbkidd Jan 19, 2024

robbkidd Jan 19, 2024

fchikwekwe Jan 19, 2024

kentquirk Jan 23, 2024

robbkidd Jan 19, 2024 •

edited

Loading

robbkidd Jan 22, 2024

kentquirk left a comment

kentquirk Jan 23, 2024

kentquirk Jan 23, 2024

kentquirk Jan 23, 2024

fchikwekwe commented Jan 25, 2024

		@@ -119,10 +119,21 @@ type HoneycombLoggerConfig struct {
		APIHost string `yaml:"APIHost" default:"https://api.honeycomb.io"`

fix: allow config bools to default to true #969

fix: allow config bools to default to true #969

Conversation

robbkidd commented Jan 18, 2024 • edited by fchikwekwe Loading

Which problem is this PR solving?

Short description of the changes

Maybe TODO

kentquirk commented Jan 18, 2024

robbkidd commented Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbkidd Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbkidd Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentquirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fchikwekwe commented Jan 25, 2024

robbkidd commented Jan 18, 2024 •

edited by fchikwekwe

Loading

robbkidd commented Jan 18, 2024 •

edited

Loading

robbkidd Jan 18, 2024 •

edited

Loading

robbkidd Jan 19, 2024 •

edited

Loading