Use logrusctx instead of logrus on task

[0x2b3bfa0]

The modifications introduced with this pull request can be used to provide detailed operation status, to avoid a a “The Cloudformation” experience where users have no insight on the operation progress.

Uses github.com/0x2b3bfa0/logrusctx, which is in turn based on github.com/juju/zaputil/zapctx, mentioned by @tasdomas in a casual conversation, unsuspecting of my implementation impetus.

Use case

logger := logrus.WithField("session", sesssionID)
logger.SetOutput(&buf)
ctx = logrusctx.WithLogger(ctx, logger)
tsk.Create(ctx)
// buf will contain the real-time progress of the operation

[dacbd]

I have a couple of questions.

1. why do we need this change?
2. can you show or say how the behavior is being changed as a result of this?
3. Is this something that could be accepted upstream to lorgrus itself?
4. perhaps using a logging library where context is supported already?

[0x2b3bfa0]

1. As stated on the pull the body, to provide [service] users with detailed operation status instead of a laconic and opaque “doing stuff, will take time” message. We don't need this change, but it seems like a nice addition to me.
2. Behavior will stay the same after merging this pull request, but it will be really easy to alter it so logs for individual requests can be stored or sent individually to requesters.
3. No, logrus is in maintenance mode and does not accept new features. See the official repository for more information.
4. Yes, that would be nice but would require us to rewrite more code, and we'd risk breaking other unorthodox use cases of the logger like the command-line tool.

See #423 (review) for context (pun intended) on the introduction of logrus to this project.

[dacbd]

1. As stated on the pull the body, to provide [service] users with detailed operation status instead of a laconic and opaque “doing stuff, will take time” message. We don't need this change, but it seems like a nice addition to me.

I'm still missing this, I did read the body. To me looks like this lets the logging write out to some buffer that we could then print out to a user via an API call (this is to help enable that correct?), but I see no changes that actually enable this, all is see is %s/ logger.Info(string)/logger.Info(ctx, string)/gc with no other changes

2. Behavior will stay the same after merging this pull request, but it will be really easy to alter it so logs for individual requests can be stored or sent individually to requesters.

Should this just go with the changes which utilize the feature?¹

3. No, logrus is in maintenance mode and does not accept new features. See the official repository for more information.

More for validity Q4?

4. Yes, that would be nice but would require us to rewrite more code, and we'd risk breaking other unorthodox use cases of the logger like the command-line tool.

Maybe I'm missing some hacky-ness somewhere but I think that swapping out the logger should be the easiest update/rewrite/refactor that we could do for this codebase?

Footnotes

1. see response to Q1 ↩

[tasdomas]

I think this is a step in the right direction. The internal task package is going to be used (hopefully) in several different contexts: in the CLI tool, in TPI and in the server. These all have different logging requirements. By adding context support to logrus we will be able to use different log writers in different setups.

[0x2b3bfa0]

I'm still missing this, I did read the body. To me looks like this lets the logging write out to some buffer that we could then print out to a user via an API call (this is to help enable that correct?), but I see no changes that actually enable this, all is see is %s/ logger.Info(string)/logger.Info(ctx, string)/gc with no other changes

Yes, this pull request just enables the possibility of injecting a custom logger into the context, but doesn't introduce any example.

Should this just go with the changes which utilize the feature?1

I would explicily like to separate both changes for pull requests like #716 (this) and #710, so they can be easily cherry-picked to the main branch and separated from commits related to the LEO server implementation.

Maybe I'm missing some hacky-ness somewhere but I think that swapping out the logger should be the easiest update/rewrite/refactor that we could do for this codebase?

Sure! What could possibly go wrong? 😅