Remove host Logger() access in favor of ctx logger #260

anuptalwalkar · 2017-02-15T02:02:14Z

No description provided.

mention-bot · 2017-02-15T02:02:15Z

@anuptalwalkar, thanks for your PR! By analyzing the history of the files in this pull request, we identified @shawnburke, @yutongp and @madhuravi to be potential reviewers.

coveralls · 2017-02-15T02:05:15Z

Coverage decreased (-0.07%) to 92.581% when pulling b69958d on with-logger into 0626e44 on master.

ascandella · 2017-02-15T02:11:18Z

Do we lose any fidelity here? Is there context on the host logger not present on ctx?

…

On Tue, Feb 14, 2017 at 6:05 PM Coveralls ***@***.***> wrote: [image: Coverage Status] <https://coveralls.io/builds/10156964> Coverage decreased (-0.07%) to 92.581% when pulling *b69958d <b69958d> on with-logger* into *0626e44 <0626e44> on master*. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#260 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7P8M438Ks-m2LIy_qOgSADXJ1KAFkks5rcl1cgaJpZM4MBOM3> .

anuptalwalkar · 2017-02-15T02:21:14Z

Nothing to lose here. We add module name to the logger today, which is now passed and available in the Logger(ctx), other than that additional fields should come from zap logger which is initialized during service setup and accessed on ulog.Logger(ctx)

ghost · 2017-02-16T06:52:48Z

modules/uhttp/filters_test.go

@@ -42,6 +41,7 @@ import (
 	"github.com/uber/jaeger-client-go/config"
 )

+<<<<<<< HEAD


rebasing/cleaning up

ghost · 2017-02-16T08:25:47Z

service/host.go

@@ -278,9 +278,9 @@ func (s *host) start() Control {

 				s.shutdownMu.Unlock()
 				if _, err := s.shutdown(e, "", nil); err != nil {
-					s.Logger().Error("Unable to shut down modules", "initialError", e, "shutdownError", err)
+					s.log.Error("Unable to shut down modules", "initialError", e, "shutdownError", err)


Looks like instead of logging here, we should wrap an error.

This is a separate error on shutdown though, it needs to be logged?

ghost · 2017-02-16T08:27:39Z

service/service_setup.go

@@ -45,7 +45,7 @@ func (svc *serviceCore) setupLogging() {
 		svc.log = logBuilder.WithConfiguration(svc.logConfig).Build()
 		ulog.SetLogger(svc.log)
 	} else {
-		svc.log.Debug("Using custom log provider due to service.WithLogger option")
+		ulog.Logger(context.Background()).Debug("Using custom log provider due to service.WithLogger option")


What does this log message even mean?

We have a service.WithLogger option during the service setup (similar to WithModule, etc). If that is set, we don't update the service.Logger

I can see this is a confusing debug message though, will update.

coveralls · 2017-02-16T20:13:21Z

Coverage decreased (-0.07%) to 92.554% when pulling 6c4e538 on with-logger into ec29e35 on master.

madhuravi · 2017-02-16T21:32:23Z

ulog/log.go

@@ -140,11 +140,13 @@ func Logger(ctx context.Context) Log {
 }

 // NewLogContext sets the context with the context aware logger
-func NewLogContext(ctx context.Context) context.Context {
+func NewLogContext(ctx context.Context, log Log) context.Context {


why pass this in explicitly? just use ulog.Logger() here

This is used when middleware sets up Log object on the context with module specific fields.

madhuravi · 2017-02-16T21:37:17Z

service/host.go

@@ -95,7 +95,7 @@ func (s *host) IsRunning() bool {
 func (s *host) OnCriticalError(err error) {
 	shutdown := true
 	if s.observer == nil {
-		s.Logger().Warn(
+		s.log.Warn(


If we decide on using the ulog.Logger, maybe we should be consistent and use it everywhere in our code. We don't have access to context here but so do service owners in some cases and we still expect them to inject a context.

I was initially thinking to use s.log internally, only for setup purpose, but you are right that we should be consistent. I'll remove log object on the host itself.

coveralls · 2017-02-16T21:55:24Z

Coverage decreased (-0.004%) to 92.618% when pulling de99f5d on with-logger into ec29e35 on master.

madhuravi · 2017-02-16T22:07:24Z

ulog/log.go

@@ -140,11 +140,13 @@ func Logger(ctx context.Context) Log {
 }

 // NewLogContext sets the context with the context aware logger
-func NewLogContext(ctx context.Context) context.Context {
+func NewLogContext(ctx context.Context, log Log) context.Context {


mild preference for method be to ContextWithLogger

more than mind here

madhuravi · 2017-02-16T22:11:06Z

modules/uhttp/http.go

 	// TODO (madhu): Add other middleware - logging, metrics.
 	module := &Module{
 		ModuleBase: *modules.NewModuleBase(mi.Name, mi.Host, []string{}),
 		handlers:   handlers,
-		fcb:        defaultFilterChainBuilder(mi.Host),
+		fcb:        defaultFilterChainBuilder(log, mi.Host.AuthClient()),


why not set ulog.SetLogger and access ulog.Logger from contextFilter?

SetLogger updates the global logger, we don't want to set it to be uhttp specific.

madhuravi · 2017-02-16T22:15:14Z

ulog/log.go

-
+	if log != nil {
+		return context.WithValue(ctx, internal.ContextLogger, log)
+	}
 	return context.WithValue(ctx, internal.ContextLogger, logger())
 }



likewise, WithTracingAware would be ContextWithTraceLogger, along those lines.

updated both.

madhuravi

Can a repo initialize two services with UberFx? How would the global ulog.Logger work in that case?

madhuravi · 2017-02-16T23:22:29Z

modules/uhttp/http.go

 	}
 	module.config = *cfg

-	module.log = module.Host().Logger().With("moduleName", mi.Name)
+	module.log = log


Is this used anywhere?

yes, logging on Start/Stop module

ghost · 2017-02-16T23:50:56Z

service/service_setup.go

-		if err != nil {
-			ulog.Logger(context.Background()).Info("Logging configuration is not provided, setting to default logger", "error", err)
-		}
+var _simpleCtx = context.Background()


nit, it is actually empty context, probably should name _emptyCtx?

Although, since it is used only for logger, can we have an _svcLogger = ulog.Logger(_simpleCtx)?

I think its better that we use ulog.Logger(_simpleCtx) in service. If we have context based objects in the future, we will have use of _simpleContext.

coveralls · 2017-02-17T00:00:55Z

Coverage decreased (-0.04%) to 92.581% when pulling 4d52a23 on with-logger into ec29e35 on master.

anuptalwalkar force-pushed the with-logger branch from b69958d to b64890e Compare February 16, 2017 01:26

ghost reviewed Feb 16, 2017

View reviewed changes

rebase from master

6c4e538

anuptalwalkar force-pushed the with-logger branch from b64890e to 6c4e538 Compare February 16, 2017 20:09

madhuravi changed the title ~~Remove hots Logger() access in favor of ctx logger~~ Remove host Logger() access in favor of ctx logger Feb 16, 2017

madhuravi reviewed Feb 16, 2017

View reviewed changes

removed log object from host

de99f5d

madhuravi reviewed Feb 16, 2017

View reviewed changes

ghost reviewed Feb 16, 2017

View reviewed changes

ghost approved these changes Feb 16, 2017

View reviewed changes

updated method names, and perform setup once

4d52a23

madhuravi approved these changes Feb 17, 2017

View reviewed changes

anuptalwalkar merged commit 3784185 into master Feb 17, 2017

anuptalwalkar deleted the with-logger branch February 17, 2017 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove host Logger() access in favor of ctx logger #260

Remove host Logger() access in favor of ctx logger #260

anuptalwalkar commented Feb 15, 2017

mention-bot commented Feb 15, 2017

coveralls commented Feb 15, 2017

ascandella commented Feb 15, 2017 via email

anuptalwalkar commented Feb 15, 2017

ghost Feb 16, 2017

anuptalwalkar Feb 16, 2017

ghost Feb 16, 2017

anuptalwalkar Feb 16, 2017

ghost Feb 16, 2017

anuptalwalkar Feb 16, 2017

anuptalwalkar Feb 16, 2017

coveralls commented Feb 16, 2017

madhuravi Feb 16, 2017

anuptalwalkar Feb 16, 2017

madhuravi Feb 16, 2017

anuptalwalkar Feb 16, 2017

coveralls commented Feb 16, 2017

madhuravi Feb 16, 2017

glibsm Feb 16, 2017

madhuravi Feb 16, 2017

anuptalwalkar Feb 16, 2017

madhuravi Feb 16, 2017

anuptalwalkar Feb 17, 2017

madhuravi left a comment

madhuravi Feb 16, 2017

anuptalwalkar Feb 17, 2017

ghost Feb 16, 2017

ghost Feb 16, 2017

anuptalwalkar Feb 17, 2017

coveralls commented Feb 17, 2017 •

edited

Remove host Logger() access in favor of ctx logger #260

Remove host Logger() access in favor of ctx logger #260

Conversation

anuptalwalkar commented Feb 15, 2017

mention-bot commented Feb 15, 2017

coveralls commented Feb 15, 2017

ascandella commented Feb 15, 2017 via email

anuptalwalkar commented Feb 15, 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

coveralls commented Feb 16, 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

coveralls commented Feb 16, 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

madhuravi 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

coveralls commented Feb 17, 2017 • edited

coveralls commented Feb 17, 2017 •

edited