-
Notifications
You must be signed in to change notification settings - Fork 50
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
[RC4] Fix for datadog logs output #196
Conversation
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!
idk if we want to add a way to have a json logger even for non DD users, I'm wondering if we're not tying KH too much with DD
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 would rather have a switch for JSON logging that something indirect based on the env.
I could then be wrapped by the config load for the prod env, but mixing the 2 is not great semantically.
If user want to use json logging, they will have to setup DD_ENV=
something, which is weird.
We have a config file, imo that should be put there:
log_format:"json"
, log_format:"text"
seems simple to implement as well.
pkg/globals/application.go
Outdated
func IsDDFormat() bool { | ||
_, isDDEnv := os.LookupEnv("DD_ENV") | ||
|
||
return isDDEnv |
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.
Not a huge fan of IsDDFormat here, what format is it? (I know it's for the logs because this PR says it, but it's a global func)
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.
ok I just reuse what was in place. Did not like the ProdEnv()
func. Will see if it is not too painful to add it to the config file instead.
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.
(actually putting this as a request change haha)
Fix to set the output for the datadog agent:
DD_ENV
env var needs to be set to activate the json outputDD_ENV
will set the datadog tagsenv