-
Notifications
You must be signed in to change notification settings - Fork 327
Avoid global configuration overriding all #655
Conversation
/cc @savaki |
trace/config.go
Outdated
// ApplyConfig applies changes to the global tracing configuration. | ||
// | ||
// Fields not provided in the given config is going to be preserved. | ||
func ApplyConfig(cfg Config) { | ||
if cfg.DefaultSampler == nil { | ||
cfg.DefaultSampler = newDefaultSampler() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you don't want to reinstall the default, you want to keep the previous value. Maybe worth having a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Nice! |
trace/config.go
Outdated
func SetConfig(cfg Config) { | ||
// ApplyConfig applies changes to the global tracing configuration. | ||
// | ||
// Fields not provided in the given config is going to be preserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo:
s/is going to/are going to/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
func TestApplyZeroConfig(t *testing.T) { | ||
cfg := config | ||
ApplyConfig(Config{}) | ||
if got, want := reflect.ValueOf(config.DefaultSampler).Pointer(), reflect.ValueOf(cfg.DefaultSampler).Pointer(); got != want { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, what is this testing for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether config.DefaultSampler and cfg.DefaultSampler pointing to the same pointer.
It will be more common for the users to tweak a few settings rather than all. Rename SetConfig to ApplyConfig and only override the non-zero values.
It will be more common for the users to tweak a few
settings rather than all.
Rename SetConfig to ApplyConfig and only
override the non-zero values.