-
Notifications
You must be signed in to change notification settings - Fork 435
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.
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
ddtrace/tracer: Add WithLogStartup startup option #860
Conversation
57e1269
to
8427f4e
Compare
8427f4e
to
377406a
Compare
377406a
to
2ddfdf8
Compare
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.
Thanks for doing this. A few nits.
ddtrace/tracer/option_test.go
Outdated
|
||
func TestWithLogStartup(t *testing.T) { | ||
var c config | ||
defaultValue := c.logStartup |
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.
We already know that the zero-value of a boolean is false, so we can simplify this and just do:
c := newConfig()
assert.False(t, c.logStartup)
WithLogStartup()(&c)
assert.True(t, c.logStartup)
That should do for the whole test. I don't think there's a need for documentation either. It's pretty self-explanatory.
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.
I agree. Inverting a value like that is a bit opaque, it's much easier to grasp the code if the values are more explicit.
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.
Thanks!
ddtrace/tracer/option.go
Outdated
@@ -443,6 +443,13 @@ func WithHostname(name string) StartOption { | |||
} | |||
} | |||
|
|||
// WithLogStartup allows control over whether to print the startup log or not | |||
func WithLogStartup(logStartup bool) StartOption { |
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.
Except maybe let's rename the argument to something else.
func WithLogStartup(logStartup bool) StartOption { | |
func WithLogStartup(yes bool) StartOption { |
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.
I'm going with enabled
to be consistent with our other boolean StartOption
s.
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.
LGTM.
Needs approval from @gbbr since I added some commits.
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.
This is fine, just a small change request to the docs please.
Co-authored-by: Gabriel Aszalos <gabriel.aszalos@gmail.com>
This PR implements the proposal described in #861