Switch logging from glog to go-kit/log #31
Conversation
There was a problem hiding this comment.
Thank for this @tdabasinskas and LGTM modulo my and @free comments.
There are some things we need to agree on within project I think:
- Os exit status. Why 255?
- Lower case or upper case log messages. I think lower case is the standard, we keep it like that in every Prometheus-based project. It's easier to read and since it is JSON at the end it does not matter much? I would vote for keeping this, but let's discuss (:
|
Thanks for the comments. I've tried addressing all of them, but let me know if I've missed something or if further changes are required. |
pkg/notify/notify.go
Outdated
| @@ -56,45 +57,45 @@ func (r *Receiver) Notify(data *alertmanager.Data) (bool, error) { | |||
| // The set of JIRA status categories is fixed, this is a safe check to make. | |||
| if issue.Fields.Status.StatusCategory.Key != "done" { | |||
| // Issue is in a "to do" or "in progress" state, all done here. | |||
| log.V(1).Infof("Issue %s for %s is unresolved, nothing to do", issue.Key, issueLabel) | |||
| level.Info(logger).Log("msg", "nothing to do as issue is unresolved", "issue", issue.Key, "label", issueLabel) | |||
There was a problem hiding this comment.
With apologies for walking back my request, I think stating the cause before the outcome makes more sense. I also find the new log messages somewhat verbose. I'm normally quite careful with grammar, but log files should be free of fluff.
pkg/notify/notify.go
Outdated
| return false, nil | ||
| } | ||
|
|
||
| resolutionTime := time.Time(issue.Fields.Resolutiondate) | ||
| if resolutionTime.Add(time.Duration(*r.conf.ReopenDuration)).After(time.Now()) { | ||
| log.Infof("Issue %s for %s was resolved on %s, reopening", issue.Key, issueLabel, resolutionTime.Format(time.RFC3339)) | ||
| return r.reopen(issue.Key) | ||
| level.Info(logger).Log("msg", "issue was resolved after reopen duration, reopening", "issue", issue.Key, "label", issueLabel, "reopenDuration", *r.conf.ReopenDuration, "resolutionTime", resolutionTime) |
Co-Authored-By: tdabasinskas <tomas@dabasinskas.net>
Co-Authored-By: tdabasinskas <tomas@dabasinskas.net>
|
Hello, @free , Sorry for the delay. Could you please let me know if any further changes are still required? I've committed your suggestions and the only two remaining comments, according to GitHub, are on outdated files. |
Co-Authored-By: tdabasinskas <tomas@dabasinskas.net>
Co-Authored-By: tdabasinskas <tomas@dabasinskas.net>
Co-Authored-By: tdabasinskas <tomas@dabasinskas.net>
Co-Authored-By: tdabasinskas <tomas@dabasinskas.net>
|
Thanks for approving it! When can we have this merged and have a new release containing these changes? |
As described under #30,
glogpackage does not support outputting logs as JSON, making it quite difficult to capture logs of JIRAlert in cloud-native deployments.The PR replaces
glogwithgo-kit/kit/logpackage, which is used both by Prometheus and Thanos. This change introduces additional command line options:--log.leveland--log.formatthat allow configuring logging verbosity and output format. In addition, all log messages were updated to use structured logging.