migrate from glog to logrus and errors to pkg/errors #98

skriss · 2017-09-14T21:31:57Z

Starting the migration to logrus. Appreciate any input before I continue across the rest of the codebase, particularly on what we should be logging at each level -- @davecheney, I know you have thoughts on the matter :)

Fixes #70

skriss · 2017-09-14T21:35:21Z

Sample output:

time="2017-09-14T21:06:38Z" level=info msg="Ensuring namespace exists for backups" namespace=heptio-ark
time="2017-09-14T21:06:38Z" level=info msg="Namespace already exists" namespace=heptio-ark
time="2017-09-14T21:06:38Z" level=info msg="Retrieving Ark configuration"
time="2017-09-14T21:06:38Z" level=info msg="Successfully retrieved Ark configuration"
time="2017-09-14T21:06:38Z" level=info msg="Using default resource priorities" priorities="[namespaces persistentvolumes persistentvolumeclaims secrets configmaps]"
time="2017-09-14T21:06:38Z" level=info msg="Configuring cloud provider for backup service"
time="2017-09-14T21:06:38Z" level=info msg="Configuring cloud provider for snapshot service"
time="2017-09-14T21:06:38Z" level=info msg="Starting controllers"
time="2017-09-14T21:06:38Z" level=info msg="Setting cloud backup cache sync period" cloudBackupCacheResyncPeriod=30m0s

jrnt30 · 2017-09-14T22:00:36Z

pkg/controller/backup_controller.go

@@ -82,7 +84,8 @@ func NewBackupController(
 		client:       client,
 		queue:        workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "backup"),

-		clock: &clock.RealClock{},
+		clock:  &clock.RealClock{},
+		logger: logrus.New(),


I'm not very familiar with the setup/configuration of logrus. Would it be more appropriate to pass this into the controllers as an argument instead of having the controllers bootstrap their own individually?

Specifically if we were to want to add in the ability to use structured logging or any of the other hooks it supports, would this be easy to accomplish?

I agree, we'll want to set up logging once, as high as we can, and pass it down.

davecheney · 2017-09-18T01:06:40Z

Review now.

davecheney

LGTM with minor comments.

TBH, I didn't review all of this as my comments were mostly the same all the way throught.

A few general comments.

Thank you! Thank you! for making logger a field, not a package level var. I cannot express how happy I am to see this.
Perhaps a convention of passing logger into the constructor/function first, rather than the last param might help. A convention might make it easier to filter out the noise the repetition of this parameter; ie, the way that ctx always comes first, logger could come second (or first if there is no context).
The remaining comments were about logging errors at info level, which is done inconsistently in this PR, sometimes errors are logged at info, sometimes at error, please try to make them consistent.
Logging an error and returning the error can lead to really verbose logs where every stack frame logs and returns the error to the caller, who logs it, and so on. To throw shade at Java, this pattern is why people rage about Java's verbosity. My general suggestion is to log the error or return it, and in most cases as the error means the function cannot continue, logging isn't an option, so you have to return it. I've also pitched my pkg/errors package as an aide for this. It's just a suggestion, i'm not shilling for my side projects.

davecheney · 2017-09-18T01:08:05Z

pkg/backup/backup.go

 			gr, err := helper.ResolveGroupResource(item)
 			if err != nil {
-				return "", err
+				ctx.log("Unable to resolve resource %q: %v", item, err)


Why did you change this from returning an error to logging the error and returning a valid value (i'm guessing "" is valid in some circumstances)

so that includes_excludes.GenerateIncludesExcludes(...) doesn't have to take a logger (right now it's glogging this error). See related change to that function to check for empty-strings.

davecheney · 2017-09-18T01:10:03Z

pkg/cloudprovider/backup_cache.go

@@ -41,15 +41,17 @@ type backupCache struct {
 	// This doesn't really need to be a map right now, but if we ever move to supporting multiple
 	// buckets, this will be ready for it.
 	buckets map[string]*backupCacheBucket
+	logger  *logrus.Logger
 }

 var _ BackupGetter = &backupCache{}


nit: I see these assertions as tests, so maybe they should move to a _test.go file. I also have a a belief that 90% of these assertions are unnecessary as tests will fail if X doesn't implement Y in the general case, and in the specific case that you want to test that X implements an Y, it should be a test case with a type assertion rather than a compilation failure.

davecheney · 2017-09-18T01:10:22Z

pkg/cloudprovider/backup_cache.go

 }

 var _ BackupGetter = &backupCache{}

 // NewBackupCache returns a new backup cache that refreshes from delegate every resyncPeriod.
-func NewBackupCache(ctx context.Context, delegate BackupGetter, resyncPeriod time.Duration) BackupGetter {
+func NewBackupCache(ctx context.Context, delegate BackupGetter, resyncPeriod time.Duration, logger *logrus.Logger) BackupGetter {


thank you from the bottom of my heart for not passing the logger in via context.WithValue.

davecheney · 2017-09-18T01:12:21Z

pkg/cloudprovider/backup_cache.go

 	c := &backupCache{
 		delegate: delegate,
 		buckets:  make(map[string]*backupCacheBucket),
+		logger:   logger,


suggestion, which you may not agree with. backupCache is unexported, and being returned inside the BackupGetter interface so if you did something like this

type backupCache struct { *logrus.Logger ... }

Then anywhere you call c.logger.Info you can use c.Info.

davecheney · 2017-09-18T01:19:39Z

pkg/cmd/server/server.go

@@ -177,12 +183,14 @@ func (s *server) loadConfig() (*api.Config, error) {
 			break
 		}
 		if !apierrors.IsNotFound(err) {
-			glog.Errorf("error retrieving configuration: %v", err)
+			s.logger.WithField("error", err).Info("Error retrieving configuration")


s/Info/Error

davecheney · 2017-09-18T01:41:32Z

pkg/controller/restore_controller.go


 	if _, err := file.Seek(0, 0); err != nil {
-		glog.V(4).Infof("error seeking: %v", err)
+		logContext.WithFields(logrus.Fields{


Please don't log an error at Info level, please don't log and return an error, just return it.

davecheney · 2017-09-18T01:42:14Z

pkg/controller/schedule_controller.go

 			return nil
 		}
-		glog.V(4).Infof("error getting schedule %s: %v", key, err)
+		logContext.WithField("error", err).Info("Error getting schedule")


Please don't log an error at Info level, please don't log and return an error, just return it.

davecheney · 2017-09-18T01:42:24Z

pkg/controller/schedule_controller.go

 	// don't modify items in the cache
 	schedule, err = cloneSchedule(schedule)
 	if err != nil {
-		glog.V(4).Infof("error cloning schedule %s: %v", key, err)
+		logContext.WithField("error", err).Info("Error cloning schedule")


davecheney · 2017-09-18T01:42:33Z

pkg/controller/schedule_controller.go

@@ -259,7 +280,7 @@ func (controller *scheduleController) processSchedule(key string) error {

 	// check for the schedule being due to run, and submit a Backup if so
 	if err := controller.submitBackupIfDue(schedule, cronSchedule); err != nil {
-		glog.V(4).Infof("error processing Schedule %v/%v: err=%v", schedule.Namespace, schedule.Name, err)
+		logContext.WithField("error", err).Info("Error processing schedule")


davecheney · 2017-09-18T01:43:00Z

pkg/controller/schedule_controller.go

+			logContext.WithFields(logrus.Fields{
+				"schedule": itm.Spec.Schedule,
+				"error":    err,
+			}).Info("Error parsing schedule")
 			validationErrors = append(validationErrors, fmt.Sprintf("invalid schedule: %v", err))


see above about maybe using something like pkg/errors to attach context to the error message, rather than using logging.

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss · 2017-09-25T17:28:23Z

@ncdc PTAL at the latest commit. This demonstrates the pattern I'd like to use for errors/logging:

-use @davecheney's pkg/errors package in order to get stack traces
-for errors returned from non-Ark libraries, wrap them (to get the stack trace and attach Ark-specific context) before returning
-when returning errors, don't also log them; the stack traces will give us the info we need
-use "structured error messages" - similar to how logrus allows appending structured key-value pairs to the end of log messages, do the same for error messages when we construct or wrap them. this allows for context to be attached to errors, and for it to be well-structured.

ncdc · 2017-09-25T18:52:06Z

pkg/controller/backup_controller.go

@@ -42,6 +42,7 @@ import (
 	arkv1client "github.com/heptio/ark/pkg/generated/clientset/typed/ark/v1"
 	informers "github.com/heptio/ark/pkg/generated/informers/externalversions/ark/v1"
 	listers "github.com/heptio/ark/pkg/generated/listers/ark/v1"
+	"github.com/heptio/ark/pkg/util"


Let's not have a util package. util/errors aliased to arkerrors or something would be better imho

yes, very much lets not have a generic util package.

ncdc · 2017-09-25T18:55:07Z

pkg/util/error.go

+		kvs = append(kvs, fmt.Sprintf("%s=%v", k, v))
+	}
+
+	return fmt.Sprintf("%s %s", message, strings.Join(kvs, " "))


Hmmm... It would be nice if the fields were exposed to logrus's WithFields() method. Maybe we need our own custom Wrap() style method that both retains the original error and maintains fields? Or something along those lines. WDYT @davecheney?

You're saying have a custom error type that contains a Fields map and when logging an errors the fields get passed to logrus?

Yeah, something along those lines. I'm not crazy about creating an error message where the string has the fields hardcoded as key=value pairs. But maybe I'm overthinking/overworrying?

I can create a custom logrus formatter to do this (which I think I need to do anyway to get the stack traces to be logged).

Before doing that let's see what @davecheney thinks

I can definitely write the stack trace to the log using a logrus hook (they allow you to permute the log Entry before it's written, so I can write the error using %+v there). This could theoretically work for errors with fields as well (though per earlier discussion, @ncdc does not want to implement this).

ncdc · 2017-10-02T20:44:57Z

@davecheney @skriss what do you think of the way that the docker/distribution project has organized its logging? To summarize:

It has its own context package that supports retrieving a logger, with or without ad-hoc fields as well as predefined context values
Major components embed a Context in the structs for component-scoped information
Individual methods can create new subcontexts with method-scoped fields

See e.g. docs at https://github.com/docker/distribution/blob/master/context/doc.go

I'm trying to avoid having to pass loggers into lots and lots of functions. I think I'd prefer they exist as part of a Context, and optionally that our major structs embed the Context for easier access.

WDYT?

davecheney · 2017-10-02T22:51:06Z

@ncdc

It has its own context package that supports retrieving a logger, with or without ad-hoc fields as well as predefined context values

meh, I have strong views on using a context value (maybe not the context.Context) to smuggle values in and out of methods. I prefer to keep things explicit, in this case passing a logger into the constructor if needed by the thing.

Major components embed a Context in the structs for component-scoped information

If this is a context.Context then that runs contrary to the advice from the Go team who recommend that a context.Context is only for request scoped variables.

(yes, I have a very complicated relationship with context.Context)

davecheney · 2017-10-02T22:58:18Z

@ncdc I'm specifically concerned with placing loggers inside a context type value because this creates invisible coupling between the code that creates the context and the code that uses it. The creator has to know to place a logger value into the context, and the code that expects a logger needs to have a fallback case if the logger value is missing. Both of these can be made more explicit by passing a logger into the constructor during construction (which most types complex enough to have something to log generally have)

ncdc · 2017-10-03T00:46:38Z

pkg/cloudprovider/backup_service.go

@@ -125,7 +127,11 @@ func (br *backupService) UploadBackup(bucket, backupName string, metadata, backu
 	// success
 	logKey := getBackupLogKey(backupName)
 	if err := br.objectStorage.PutObject(bucket, logKey, log); err != nil {
-		glog.Errorf("error uploading %s/%s: %v", bucket, logKey, err)
+


delete blank line

ncdc · 2017-10-03T00:47:24Z

pkg/cloudprovider/backup_service.go

+
+		br.logger.WithError(err).WithFields(logrus.Fields{
+			"bucket": bucket,
+			"logKey": logKey,


Just call this key?

ncdc · 2017-10-03T00:47:36Z

pkg/cloudprovider/backup_service.go

@@ -149,7 +155,7 @@ func (br *backupService) GetAllBackups(bucket string) ([]*api.Backup, error) {
 	for _, backupDir := range prefixes {
 		backup, err := br.GetBackup(bucket, backupDir)
 		if err != nil {
-			glog.Errorf("Error reading backup directory %s: %v", backupDir, err)
+			br.logger.WithError(err).WithField("backupDir", backupDir).Error("Error reading backup directory")


s/backupDir/dir/

ncdc · 2017-10-03T00:51:46Z

pkg/cmd/server/server.go


 	ctx := s.ctx
 	var wg sync.WaitGroup

 	cloudBackupCacheResyncPeriod := durationMin(config.GCSyncPeriod.Duration, config.BackupSyncPeriod.Duration)
-	glog.Infof("Caching cloud backups every %s", cloudBackupCacheResyncPeriod)
+	s.logger.WithField("cloudBackupCacheResyncPeriod", cloudBackupCacheResyncPeriod).Info("Setting cloud backup cache sync period")


I'd change cloudBackupCacheResyncPeriod to one of period, resyncPeriod, duration. Trying to avoid repetition.

ncdc · 2017-10-03T00:53:18Z

pkg/controller/backup_controller.go

@@ -215,11 +228,11 @@ func (controller *backupController) processBackup(key string) error {
 		return nil
 	}

-	glog.V(4).Infof("Cloning backup %s", key)
+	logContext.Debug("Cloning backup")


It was V4, which is essentially debug

ncdc · 2017-10-03T00:58:51Z

pkg/controller/download_request_controller.go

@@ -101,7 +99,7 @@ func (c *downloadRequestController) Run(ctx context.Context, numWorkers int) err
 	var wg sync.WaitGroup

 	defer func() {
-		glog.Infof("Waiting for workers to finish their work")
+		c.logger.Info("Waiting for workers to finish their work")


This should be consistent with all the other controllers. The intent is to log that we're waiting, then shut down the queue so we're actually doing so.

ncdc · 2017-10-03T00:59:30Z

pkg/controller/gc_controller.go

 		if err := c.snapshotService.DeleteSnapshot(volumeBackup.SnapshotID); err != nil {
-			glog.Errorf("error deleting snapshot %v: %v", volumeBackup.SnapshotID, err)
+			logContext.WithError(err).WithField("snapshotID", volumeBackup.SnapshotID).Error("Error deleting snapshot")


isn't the field already part of the logContext?

ncdc · 2017-10-03T00:59:40Z

pkg/controller/gc_controller.go

@@ -121,45 +125,46 @@ func (c *gcController) garbageCollectBackup(backup *api.Backup, deleteBackupFile
 	deletionFailure := false

 	for _, volumeBackup := range backup.Status.VolumeBackups {
-		glog.Infof("Removing snapshot %s associated with backup %s", volumeBackup.SnapshotID, kube.NamespaceAndName(backup))
+		logContext.WithField("snapshotID", volumeBackup.SnapshotID).Info("Removing snapshot associated with backup")


call the field id

ncdc · 2017-10-03T01:01:24Z

pkg/controller/restore_controller.go

 		return err
 	}

-	glog.V(4).Infof("Getting restore %s", key)
+	logContext.Debug("Getting Restore")


Same comment as above - yes - we basically want to lump Infof/V1/V2 as Info and V3+ as Debug

ncdc · 2017-10-03T01:02:29Z

pkg/controller/restore_controller.go

 		errors.Ark = append(errors.Ark, err.Error())
 		return
 	}

 	logFile, err := ioutil.TempFile("", "")
 	if err != nil {
-		glog.Errorf("error creating log temp file: %v", err)
+		logContext.WithField("error", err).Error("Error creating log temp file")
 		errors.Ark = append(errors.Ark, err.Error())
 		return
 	}

 	defer func() {
 		if err := tmpFile.Close(); err != nil {


Do you have suggestions for how to do this better? I'm not sure we can do much other than throw our hands up in the air?

ncdc · 2017-10-03T13:50:15Z

@skriss I talked with @davecheney on slack last night and we're both good with the approach you have here. I think what's left before we merge this:

remove the wrapped error thing
implement the logrus hook so you can custom format the error stack
ensure anything that was V3 or higher is now Debug
ensure anything that was V or lower is now Info

skriss · 2017-10-03T20:41:36Z

pushed updates. recommended to look at the commits separately, as one is focused on logging with the controllers, while another wraps external library errors across all non-controller code. I think the main issue now is to figure out if/when we want stack traces, and if we don't want full ones, what we do want and how to get it.

skriss · 2017-10-03T23:01:10Z

Looks like we can't get nicely-formatted stack traces without a custom logrus formatter (which is more work than the hook that I implemented) -- see sirupsen/logrus#608

ncdc · 2017-10-04T15:12:11Z

FYI we can get the file, line, and function name of the error if we always use github.com/pkg/errors to create errors (no more fmt.Errorf etc.):

type stackTracker interface {
  StackTrace() errors.StackTrace
}

err := someFunction()
stackErr, ok := errors.Cause(err).(stackTracker)
if ok {
  stackTrace := stackErr.StackTrace()
  functionName := fmt.Sprintf("%n", stackTrace[0])
  fileAndLine := fmt.Sprintf("%s:%d", stackTrace[0], stackTrace[0])
  logrus.WithError(err).WithFields(logrus.Fields{
    "error.file": fileAndLine,
    "error.function": functionName,
  }).Error("something bad happened!")
}

This prints out something like this (I have a few functions that call errors.Wrap to see the effect):

time="2017-10-04T11:04:40-04:00" level=error msg="something bad happened!" error="from a: from b: wrap in c: andy" error.file="main.go:24" error.function=c

The only thing I wish github.com/pkg/errors did was allow us to get the full path of the file, but the only way to do that is with %+s, but that unfortunately prints out like this:

main.c
	/Users/andy/go/src/github.com/ncdc/wraperrorslogrus/main.go

and I don't want the function name or the newline+tab formatting. @davecheney any chance we could just expose a FileLine method on Frame? Alternatively since Frame is just a typedef'd uintptr, we can manually call fn := runtime.FuncForPC() and fn.FileLine(), but I think modifying github.com/pkg/errors would be better. WDYT?

ncdc · 2017-10-04T16:14:02Z

Minor correction to determining the innermost error location:

func getInnermostTrace(err error) stackTracer {
  var tracer stackTracer
  for {
    t, isTracer := err.(stackTracer)
    if isTracer {
      tracer = t
    }

    c, isCauser := err.(causer)
    if isCauser {
      err = c.Cause()
    }
    if !isTracer && !isCauser {
      return tracer
    }

  }
  return nil
}

ncdc · 2017-10-04T16:22:52Z

So the guidelines are probably:

If you are creating a new error, use one of the methods from github.com/pkg/errors (New or Errorf)
If you are returning an error you got from a function call
- Use errors.Wrap if you want to record both the stack trace and a custom error message
- Use errors.WithStack if you want to record the stack trace but no custom error message
- Do not use errors.WithMessage- if the underlying error didn't come from github.com/pkg/errors, then it might not be possible to record the innermost stack frame where the error originated

WDYT?

skriss · 2017-10-04T16:25:32Z

cool, this looks great. guidelines make sense -- I've already done both. Re: the second, I would add that you should call .Wrap or .WithStack for errors that are being returned from third-party libraries, but not for errors returning from your code (since if you do bullet 1 they'll already have a stack trace)

ncdc · 2017-10-04T16:28:03Z

Is there any harm in wrapping an error that came from github.com/pkg/errors.New()?

skriss · 2017-10-04T16:31:13Z

well, every time you wrap you get a new stack trace from that point, which can result in the duplicated stacks like we saw yesterday. will check with the updated logging approach.

ncdc · 2017-10-04T16:33:03Z

Right, if you print a full trace of the error, repeated wraps leads to repeated traces printed out. Ok, let's not do that :).

ncdc · 2017-10-04T16:38:01Z

I wonder if we need 2 sets of file/function fields: 1 for the innermost error, and 1 for where it's actually logged from?

davecheney · 2017-10-04T21:34:04Z

Marking my changes as approved; I don't want to be a blocker on landing this change.

ncdc · 2017-10-04T17:12:38Z

pkg/cloudprovider/azure/block_storage_adapter.go

@@ -85,7 +85,7 @@ func NewBlockStorageAdapter(location string, apiTimeout time.Duration) (cloudpro

 	spt, err := helpers.NewServicePrincipalTokenFromCredentials(cfg, azure.PublicCloud.ResourceManagerEndpoint)
 	if err != nil {
-		return nil, fmt.Errorf("error creating new service principal: %v", err)
+		return nil, errors.WithStack(err)


Do you want to preserve the message?

ncdc · 2017-10-04T17:14:12Z

pkg/cloudprovider/backup_cache.go

@@ -64,10 +66,10 @@ func (c *backupCache) refresh() {
 	c.lock.Lock()
 	defer c.lock.Unlock()

-	glog.V(4).Infof("refreshing all cached backup lists from object storage")
+	c.logger.Info("refreshing all cached backup lists from object storage")


ncdc · 2017-10-04T17:14:19Z

pkg/cloudprovider/backup_cache.go


 	for bucketName, bucket := range c.buckets {
-		glog.V(4).Infof("refreshing bucket %q", bucketName)
+		c.logger.WithField("bucket", bucketName).Info("Refreshing bucket")


ncdc · 2017-10-04T17:14:26Z

pkg/cloudprovider/backup_cache.go

 	if found {
-		glog.V(4).Infof("returning cached backup list for bucket %q", bucketName)
+		logContext.Info("Returning cached backup list for bucket")


remove "for bucket"

ncdc · 2017-10-04T17:14:35Z

pkg/cloudprovider/backup_cache.go

@@ -76,12 +78,15 @@ func (c *backupCache) GetAllBackups(bucketName string) ([]*v1.Backup, error) {
 	c.lock.RLock()
 	bucket, found := c.buckets[bucketName]
 	c.lock.RUnlock()
+
+	logContext := c.logger.WithField("bucketName", bucketName)


s/bucketName/bucket/

ncdc · 2017-10-04T21:34:26Z

pkg/restore/restorers/pod_restorer.go

@@ -82,10 +85,10 @@ func (nsr *podRestorer) Prepare(obj runtime.Unstructured, restore *api.Restore,
 		return nil, nil, err
 	}

-	glog.V(4).Infof("setting spec.volumes")
+	r.logger.Info("Setting spec.volumes")


ncdc · 2017-10-04T21:34:31Z

pkg/restore/restorers/pod_restorer.go

 	spec["volumes"] = newVolumes

-	glog.V(4).Infof("iterating over containers")
+	r.logger.Info("iterating over containers")


ncdc · 2017-10-04T21:35:20Z

pkg/restore/restorers/pod_restorer.go

@@ -94,13 +97,12 @@ func (nsr *podRestorer) Prepare(obj runtime.Unstructured, restore *api.Restore,
 				return err
 			}

-			glog.V(4).Infof("checking volumeMount with name %q", name)
-
+			r.logger.WithField("volumeMount", name).Info("Checking volumeMount")


ncdc · 2017-10-04T21:35:23Z

pkg/restore/restorers/pod_restorer.go

 			if !defaultTokenRegex.MatchString(name) {
-				glog.V(4).Infof("preserving volumeMount")
+				r.logger.WithField("volumeMount", name).Info("Preserving volumeMount")


ncdc · 2017-10-04T21:35:27Z

pkg/restore/restorers/pod_restorer.go

 				newVolumeMounts = append(newVolumeMounts, volumeMount)
 			} else {
-				glog.V(4).Infof("excluding volumeMount")
+				r.logger.WithField("volumeMount", name).Info("Excluding volumeMount")


skriss · 2017-10-04T22:05:57Z

@ncdc addressed most of the feedback. I'll squash this down to two commits for the 2 new vendored dependencies, one commit for the logrus hook and one commit for the rest.

ncdc · 2017-10-05T13:37:01Z

@skriss squash & let's get it in. 🎉

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss · 2017-10-05T15:52:37Z

squashed, building now...

skriss · 2017-10-05T15:59:16Z

@ncdc good to go!

bug 1967858 - bug introduced in 1.6 restore progress: fix CR restore

skriss requested review from ncdc and davecheney September 14, 2017 21:31

jrnt30 reviewed Sep 14, 2017

View reviewed changes

skriss force-pushed the logrus branch from 616976e to 51666a9 Compare September 15, 2017 18:55

davecheney suggested changes Sep 18, 2017

View reviewed changes

skriss force-pushed the logrus branch from 02facc1 to 71958ab Compare September 25, 2017 17:22

vendor logrus

2683ead

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss force-pushed the logrus branch from 71958ab to 573fd1d Compare September 25, 2017 17:23

skriss force-pushed the logrus branch from 573fd1d to 301f9b5 Compare September 25, 2017 17:33

ncdc reviewed Sep 25, 2017

View reviewed changes

skriss force-pushed the logrus branch from 301f9b5 to 6748065 Compare September 25, 2017 19:39

ncdc self-assigned this Sep 26, 2017

ncdc reviewed Oct 3, 2017

View reviewed changes

skriss force-pushed the logrus branch from 6748065 to ead3725 Compare October 3, 2017 20:39

davecheney approved these changes Oct 4, 2017

View reviewed changes

ncdc suggested changes Oct 4, 2017

View reviewed changes

skriss added 3 commits October 5, 2017 08:46

vendor pkg/errors

791a06d

Signed-off-by: Steve Kriss <steve@heptio.com>

switch logging to logrus and errors to pkg/errors

9141650

Signed-off-by: Steve Kriss <steve@heptio.com>

add logrus hook to log error file/line/function from stack traces

9ac71b0

Signed-off-by: Steve Kriss <steve@heptio.com>

skriss force-pushed the logrus branch from 5865b34 to 9ac71b0 Compare October 5, 2017 15:52

skriss changed the title ~~[WIP] migrate from glog to logrus~~ migrate from glog to logrus and errors to pkg/errors Oct 5, 2017

skriss added this to the v0.5.0 milestone Oct 5, 2017

ncdc added the enhancement label Oct 5, 2017

ncdc approved these changes Oct 5, 2017

View reviewed changes

ncdc merged commit 87f60ed into vmware-tanzu:master Oct 5, 2017

skriss deleted the logrus branch October 5, 2017 18:42

jmontleon pushed a commit to jmontleon/velero that referenced this pull request Jul 7, 2021

Merge pull request vmware-tanzu#98 from sseago/crd-preprocessing

edb8c88

bug 1967858 - bug introduced in 1.6 restore progress: fix CR restore

migrate from glog to logrus and errors to pkg/errors #98

migrate from glog to logrus and errors to pkg/errors #98

Conversation

skriss commented Sep 14, 2017 • edited Loading

skriss commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecheney commented Sep 18, 2017

davecheney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Oct 2, 2017

davecheney commented Oct 2, 2017

davecheney commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Oct 3, 2017

skriss commented Oct 3, 2017

skriss commented Oct 3, 2017

ncdc commented Oct 4, 2017

ncdc commented Oct 4, 2017

ncdc commented Oct 4, 2017

skriss commented Oct 4, 2017

ncdc commented Oct 4, 2017

skriss commented Oct 4, 2017

ncdc commented Oct 4, 2017

ncdc commented Oct 4, 2017

davecheney commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Oct 4, 2017

ncdc commented Oct 5, 2017

skriss commented Oct 5, 2017

skriss commented Oct 5, 2017

skriss commented Sep 14, 2017 •

edited

Loading