Skip to content

Commit

Permalink
apply suggestions from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@sywhang
sywhang committed Mar 22, 2022
1 parent 466d9df commit aec2b2a
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 26 deletions.
2 changes: 1 addition & 1 deletion app_test.go
View file
Expand Up @@ -423,7 +423,7 @@ func TestWithLoggerErrorUseDefault(t *testing.T) {
// Failed: must provide constructor function, got (type *bytes.Buffer)

out := string(stderr)
assert.Contains(t, out, "[Fx] SUPPLY\t*zap.Logger from Module \"\"\n")
assert.Contains(t, out, "[Fx] SUPPLY\t*zap.Logger\n")
assert.Contains(t, out, "[Fx] ERROR\t\tFailed to initialize custom logger: fx.WithLogger")
assert.Contains(t, out, "must provide constructor function, got (type *bytes.Buffer)\n")
}
Expand Down
22 changes: 18 additions & 4 deletions fxevent/console.go
View file
Expand Up @@ -62,25 +62,39 @@ func (l *ConsoleLogger) LogEvent(event Event) {
case *Supplied:
if e.Err != nil {
l.logf("ERROR\tFailed to supply %v: %v", e.TypeName, e.Err)
} else if e.ModuleName != "" {
l.logf("SUPPLY\t%v from module %q", e.TypeName, e.ModuleName)
} else {
l.logf("SUPPLY\t%v from Module \"%v\"", e.TypeName, e.ModuleName)
l.logf("SUPPLY\t%v", e.TypeName, e.ModuleName)
}
case *Provided:
for _, rtype := range e.OutputTypeNames {
l.logf("PROVIDE\t%v <= %v from Module \"%v\"", rtype, e.ConstructorName, e.ModuleName)
if e.ModuleName != "" {
l.logf("PROVIDE\t%v <= %v from module %q", rtype, e.ConstructorName, e.ModuleName)
} else {
l.logf("PROVIDE\t%v <= %v", rtype, e.ConstructorName)
}
}
if e.Err != nil {
l.logf("Error after options were applied: %v", e.Err)
}
case *Decorated:
for _, rtype := range e.OutputTypeNames {
l.logf("DECORATE\t%v <= %v from Module \"%v\"", rtype, e.DecoratorName, e.ModuleName)
if e.ModuleName != "" {
l.logf("DECORATE\t%v <= %v from module %q", rtype, e.DecoratorName, e.ModuleName)
} else {
l.logf("DECORATE\t%v <= %v", rtype, e.DecoratorName)
}
}
if e.Err != nil {
l.logf("Error after options were applied: %v", e.Err)
}
case *Invoking:
l.logf("INVOKE\t\t%s from Module \"%v\"", e.FunctionName, e.ModuleName)
if e.ModuleName != "" {
l.logf("INVOKE\t\t%s from module %q", e.FunctionName, e.ModuleName)
} else {
l.logf("INVOKE\t\t%s", e.FunctionName)
}
case *Invoked:
if e.Err != nil {
l.logf("ERROR\t\tfx.Invoke(%v) called from:\n%+vFailed: %v", e.FunctionName, e.Trace, e.Err)
Expand Down
8 changes: 4 additions & 4 deletions fxevent/console_test.go
View file
Expand Up @@ -100,7 +100,7 @@ func TestConsoleLogger(t *testing.T) {
{
name: "Supplied",
give: &Supplied{TypeName: "*bytes.Buffer", ModuleName: "myModule"},
want: "[Fx] SUPPLY *bytes.Buffer from Module \"myModule\"\n",
want: "[Fx] SUPPLY *bytes.Buffer from module \"myModule\"\n",
},
{
name: "SuppliedError",
Expand All @@ -114,7 +114,7 @@ func TestConsoleLogger(t *testing.T) {
ModuleName: "myModule",
OutputTypeNames: []string{"*bytes.Buffer"},
},
want: "[Fx] PROVIDE *bytes.Buffer <= bytes.NewBuffer() from Module \"myModule\"\n",
want: "[Fx] PROVIDE *bytes.Buffer <= bytes.NewBuffer() from module \"myModule\"\n",
},
{
name: "Decorated",
Expand All @@ -123,7 +123,7 @@ func TestConsoleLogger(t *testing.T) {
ModuleName: "myModule",
OutputTypeNames: []string{"*bytes.Buffer"},
},
want: "[Fx] DECORATE *bytes.Buffer <= bytes.NewBuffer() from Module \"myModule\"\n",
want: "[Fx] DECORATE *bytes.Buffer <= bytes.NewBuffer() from module \"myModule\"\n",
},
{
name: "DecorateError",
Expand All @@ -133,7 +133,7 @@ func TestConsoleLogger(t *testing.T) {
{
name: "Invoking",
give: &Invoking{FunctionName: "bytes.NewBuffer()"},
want: "[Fx] INVOKE bytes.NewBuffer() from Module \"\"\n",
want: "[Fx] INVOKE bytes.NewBuffer()\n",
},
{
name: "Invoked/Error",
Expand Down
21 changes: 14 additions & 7 deletions fxevent/zap.go
View file
Expand Up @@ -77,47 +77,47 @@ func (l *ZapLogger) LogEvent(event Event) {
case *Supplied:
l.Logger.Info("supplied",
zap.String("type", e.TypeName),
zap.String("module", e.ModuleName),
moduleField(e.ModuleName),
zap.Error(e.Err))
case *Provided:
for _, rtype := range e.OutputTypeNames {
l.Logger.Info("provided",
zap.String("constructor", e.ConstructorName),
zap.String("module", e.ModuleName),
moduleField(e.ModuleName),
zap.String("type", rtype),
)
}
if e.Err != nil {
l.Logger.Error("error encountered while applying options",
zap.String("module", e.ModuleName),
moduleField(e.ModuleName),
zap.Error(e.Err))
}
case *Decorated:
for _, rtype := range e.OutputTypeNames {
l.Logger.Info("decorated",
zap.String("decorator", e.DecoratorName),
zap.String("module", e.ModuleName),
moduleField(e.ModuleName),
zap.String("type", rtype),
)
}
if e.Err != nil {
l.Logger.Error("error encountered while applying options",
zap.String("module", e.ModuleName),
moduleField(e.ModuleName),
zap.Error(e.Err))
}
case *Invoking:
// Do not log stack as it will make logs hard to read.
l.Logger.Info("invoking",
zap.String("function", e.FunctionName),
zap.String("module", e.ModuleName),
moduleField(e.ModuleName),
)
case *Invoked:
if e.Err != nil {
l.Logger.Error("invoke failed",
zap.Error(e.Err),
zap.String("stack", e.Trace),
zap.String("function", e.FunctionName),
zap.String("module", e.ModuleName),
moduleField(e.ModuleName),
)
}
case *Stopping:
Expand Down Expand Up @@ -147,3 +147,10 @@ func (l *ZapLogger) LogEvent(event Event) {
}
}
}

func moduleField(name string) zap.Field {
if len(name) == 0 {
return zap.Skip()
}
return zap.String("module", name)
}
15 changes: 5 additions & 10 deletions fxevent/zap_test.go
View file
Expand Up @@ -131,18 +131,16 @@ func TestZapLogger(t *testing.T) {
give: &Supplied{TypeName: "*bytes.Buffer"},
wantMessage: "supplied",
wantFields: map[string]interface{}{
"type": "*bytes.Buffer",
"module": "",
"type": "*bytes.Buffer",
},
},
{
name: "SuppliedError",
give: &Supplied{TypeName: "*bytes.Buffer", Err: someError},
wantMessage: "supplied",
wantFields: map[string]interface{}{
"type": "*bytes.Buffer",
"module": "",
"error": "some error",
"type": "*bytes.Buffer",
"error": "some error",
},
},
{
Expand All @@ -164,8 +162,7 @@ func TestZapLogger(t *testing.T) {
give: &Provided{Err: someError},
wantMessage: "error encountered while applying options",
wantFields: map[string]interface{}{
"error": "some error",
"module": "",
"error": "some error",
},
},
{
Expand All @@ -187,8 +184,7 @@ func TestZapLogger(t *testing.T) {
give: &Decorated{Err: someError},
wantMessage: "error encountered while applying options",
wantFields: map[string]interface{}{
"error": "some error",
"module": "",
"error": "some error",
},
},
{
Expand All @@ -208,7 +204,6 @@ func TestZapLogger(t *testing.T) {
"error": "some error",
"stack": "",
"function": "bytes.NewBuffer()",
"module": "",
},
},
{
Expand Down

0 comments on commit aec2b2a

Please sign in to comment.