Structured logging & related refactoring#998
Conversation
mdemirhan
commented
May 30, 2018
mdemirhan
commented
- Refactor the remaining code under /pkg to use structured logging
- Split elaconfig.yaml into multiple files, grouped by subject area - domain, autoscale, observability and elafros' own logging settings each get their own config files.
- Every component now shares a common logging configuration and an override-able level
- Change the verbose messages to Debug level in autoscaler
… domain, autoscale, observability and elafros' own logging settings each get their own config files. * Refactor the remaining code under /pkg to use structured logging * Every component now shares a common logging configuration and an override-able level * Change the verbose messages to Debug level in autoscaler
mdemirhan
requested review from
akyyy,
grantr,
josephburnett,
tcnghia and
yanweiguo
google-prow-robot
added
the
size/XXL
|
/test pull-elafros-elafros-build-tests |
| _, err = dc.Update(deployment) | ||
| if err != nil { | ||
| glog.Errorf("Error updating Deployment %q: %s", elaDeployment, err) | ||
| logger.Errorf("Error updating Deployment %q: %s", elaDeployment, err) |
There was a problem hiding this comment.
Nit: You changed all other err to be zap.Error(err). missed this one?
There was a problem hiding this comment.
This one is slightly different: it is using string formatting (Errorf instead of Error).
| if elaNamespace == "" { | ||
| logger.Fatal("No ELA_NAMESPACE provided.") | ||
| } | ||
| logger.Infof("ELA_REVISION=%v", elaNamespace) |
There was a problem hiding this comment.
logger.Infof("ELA_NAMESPACE=%v", elaNamespace)
| func main() { | ||
| // Even though we have no flags, glog has some hence requiring | ||
| // flag.Parse(). | ||
| flag.Parse() |
There was a problem hiding this comment.
Is this line still needed?
flag.Parse()
There was a problem hiding this comment.
Nope. Removed. Thanks for the catch!
| // LoggingURLTemplate is a string containing the logging url template where | ||
| // the variable REVISION_UID will be replaced with the created revision's UID. | ||
| LoggingURLTemplate string | ||
|
|
There was a problem hiding this comment.
It seems line 115 in this file still has
// see (elaconfig.yaml)
|
/lgtm |
|
/assign @josephburnett Joe, can you please take a look as well? This changes quite a bit in autoscaler and I want to make sure everyone working in autoscaler gets their say before I check this in :) |
| // flag.Parse(). | ||
| flag.Parse() | ||
| glog.Info("Queue container is running") | ||
| logger = logging.NewLogger(os.Getenv("ELA_LOGGING_CONFIG"), os.Getenv("ELA_LOGGING_LEVEL")).Named("ela-queueproxy") |
There was a problem hiding this comment.
You might want to rebase this and get rid of all "ela" strings since we renamed to "knative". I prefer not using prefixes at all (e.g. "ela-" or "knative-"), since it's fairly redundant.
There was a problem hiding this comment.
I would rather check this PR in and do the renaming in a separate PR as there are a lot more "ela" prefixed items than just some environment variables. Let me know if that sounds good.
| ":config-domain", | ||
| ":config-autoscaler", | ||
| ":config-logging", | ||
| ":config-observability", |
There was a problem hiding this comment.
What is the difference between "logging" and "observability"? It seems observability has a bunch of logging configuration in it? I'm probably just missing a subtle distinction here.
There was a problem hiding this comment.
logging is the logging configuration of our own code and typically only useful for contributors. Observability is the logging + metrics + tracing configuration we expose to cluster operators (i.e. the public facing configuration of observability work group).
|
/assign @mattmoor |
|
/approve For Gopkg.lock |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, mdemirhan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-prow-robot
added
the
approved
|
/lgtm |
5 participants
