Allow fx.Module-specific custom loggers #947

tchung1118 · 2022-09-28T19:03:47Z

Currently passing fx.WithLogger option to fx.Module does not work because fx currently does not support specifying module-level custom loggers.

Now fx.WithLogger can be passed to fx.Modules for specifying custom loggers for fx.Modules. Also added some test cases to test fx.Module-specific loggers.

Fixes #894

Currently passing fx.WithLogger option to fx.Module does not work because fx currently does not support specifying module-level custom loggers. Now fx.WithLogger can be passed to fx.Modules for specifying custom loggers for fx.Modules Fixes uber-go#894

CLAassistant · 2022-09-28T19:03:52Z

All committers have signed the CLA.

codecov · 2022-09-28T19:07:43Z

Codecov Report

Merging #947 (2e4bbbb) into master (dafa38f) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #947      +/-   ##
==========================================
+ Coverage   98.72%   98.89%   +0.17%     
==========================================
  Files          30       30              
  Lines        1332     1360      +28     
==========================================
+ Hits         1315     1345      +30     
+ Misses         11       10       -1     
+ Partials        6        5       -1

Impacted Files	Coverage Δ
app.go	`95.08% <100.00%> (+0.04%)`	⬆️
module.go	`100.00% <100.00%> (ø)`
log.go	`100.00% <0.00%> (+28.57%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

- added another failure case for WithLogger in fx.Module - updated existing failure cases to expect any fx.Invokes to be not called

sywhang · 2022-09-29T17:48:55Z

module.go

+	m.log.LogEvent(ev)
+}
+
+func (m *module) constructAllCustomLoggers() {


let's add a comment here clarifying that this will build the custom loggers for all the modules in the tree.

Added a comment

sywhang · 2022-09-29T17:50:04Z

module.go

+		buffer, ok := m.log.(*logBuffer)
+		if ok {


You can inline this:

Suggested change

buffer, ok := m.log.(*logBuffer)

if ok {

if buffer, ok := m.log.(*logBuffer); ok {

See also: https://github.com/uber-go/guide/blob/master/style.md#reduce-scope-of-variables

sywhang · 2022-09-29T17:50:53Z

module.go

+	if err := m.scope.Provide(p.Target); err != nil {
+		return fmt.Errorf("fx.WithLogger(%v) from:\n%+vFailed: %v",
+			fname, p.Stack, err)
+	}


add the module name in this error. Also, use the %w modifier to format the error type to make it print nicely.

Updated the error message to include module name

module_test.go

sywhang · 2022-09-29T18:04:32Z

module_test.go

+
+		require.NoError(t, app.Err())
+
+		assert.Equal(t, []string{"Started", "Stopped"}, spy.EventTypes())


These tests may actually be better off written as a table test.
See: https://github.com/uber-go/guide/blob/master/style.md#test-tables

sywhang · 2022-09-29T18:08:27Z

module_test.go

+		var buff bytes.Buffer
+		app := fx.New(
+			redis,
+			fx.Logger(log.New(&buff, "", 0)),


this is deprecated - can you change these with WithLogger and use spy like the other tests?

- added comments on module.constructAllCustomLoggers - made module's parent logger the default logger for any given module - coding style changes per style guide - added more test cases for module-specific logger

sywhang · 2022-09-29T22:21:01Z

module_test.go

+						fx.Invoke(func(s string) {
+							assert.Fail(t, "this should never run")
+						}),
+					)...,


collect the default options into fx.Options instead of appending and exploding it like this.

sywhang · 2022-09-29T22:22:33Z

module_test.go

+			},
+			{
+				desc: "logger dependency in module failed to build",
+				giveModuleOpts: []fx.Option{


Suggested change

giveModuleOpts: []fx.Option{

giveModuleOpts: fx.Options(

sywhang · 2022-09-30T02:46:41Z

module.go

+	}()
+
+	if err := m.scope.Provide(p.Target); err != nil {
+		return fmt.Errorf("fx.WithLogger(%v) from:\n%+v\nin Module: %v\nFailed: %w",


Use %q to put the module's name in quotes in the error string.

Suggested change

return fmt.Errorf("fx.WithLogger(%v) from:\n%+v\nin Module: %v\nFailed: %w",

return fmt.Errorf("fx.WithLogger(%v) from:\n%+v\nin Module: %q\nFailed: %w",

sywhang · 2022-09-30T02:47:39Z

module.go

+	// TODO: Use dig.FillProvideInfo to inspect the provided constructor
+	// and fail the application if its signature didn't match.
+


I believe this comment is misplaced. It should be placed above the m.scope.Provide, not here.

sywhang · 2022-09-30T03:43:57Z

module_test.go

+			fx.Provide(func() *Logger {
+				return &Logger{Name: "grandchild"}
+			}),
+			fx.Invoke(func(r *Logger) {
+				assert.Equal(t, "grandchild", r.Name)
+			}),


I had to stare at this test for a while because it wasn't clear what this Logger thing was. Looks like this is just a random dependency that gets Provided to make sure that the "Provided" event gets logged by the loggers you supply, which are appSpy and childSupply.

Can you change this so that it uses some other naive looking type, and not something that looks like an actual logger that gets passed to fx.WithLogger?

sywhang · 2022-09-30T03:45:49Z

module_test.go

+			giveModuleOpts  fx.Option
+			giveAppOpts     fx.Option
+			wantErrContains []string
+			wantEventTypes  []string


nit: :%s/wantEventTypes/wantEvents/g

sywhang · 2022-09-30T03:47:39Z

module_test.go

+					assert.Contains(t,
+						err.Error(),
+						contains,
+					)


Suggested change

assert.Contains(t,

err.Error(),

contains,

)

assert.Contains(t, err.Error(), contains)

sywhang · 2022-09-30T03:48:47Z

module_test.go

+		// events from modules do not appear in app logger
+		assert.Equal(t, []string{
+			"Provided", "Provided", "Provided",
+			"LoggerInitialized", "Invoking", "Invoked",
+		}, appSpy.EventTypes())


Add what you have as a comment as the last parameter of assert.Equal and it'll show up in the test failure message if it passes. i.e.

Suggested change

// events from modules do not appear in app logger

assert.Equal(t, []string{

"Provided", "Provided", "Provided",

"LoggerInitialized", "Invoking", "Invoked",

}, appSpy.EventTypes())

// events from modules do not appear in app logger

assert.Equal(t, []string{

"Provided", "Provided", "Provided",

"LoggerInitialized", "Invoking", "Invoked",

}, appSpy.EventTypes(), "events from modules should not appear in app logger")

- Logger switched to another type for WithLogger test cases - used %q for module name in error message - comments updated to be clearer - more test case cleanup

sywhang · 2022-10-02T20:02:10Z

module.go

+	}
+}
+
+// Mirroring the behavior of app.constructCustomLogger


We're copy-pasting a lot of code here. Looking at app.constructCustomLogger, we can reuse this method and remove the duplication.

i.e. the App struct already has a "root" scope module. You should be able to just use that module to construct the custom loggers using this method and remove the app.constructCustomLogger method, rather than duplicating the feature across the two types.

To do this, I would need to refactor the whole initialization process of app.log to be handled by module instead of fx.New. Is it reasonable to include this type of change in this PR's scope?

You can file a follow-up task to refactor it if you believe this PR is getting too large.

GO-1690 created

module_test.go

…vents

module_test.go

* Allow fx.Module-specific custom loggers Currently passing fx.WithLogger option to fx.Module does not work because fx currently does not support specifying module-level custom loggers. Now fx.WithLogger can be passed to fx.Modules for specifying custom loggers for fx.Modules Fixes uber-go#894 * Adding more test cases for module-level logger * More test case cleanups: - added another failure case for WithLogger in fx.Module - updated existing failure cases to expect any fx.Invokes to be not called * Addressing comments from PR review - added comments on module.constructAllCustomLoggers - made module's parent logger the default logger for any given module - coding style changes per style guide - added more test cases for module-specific logger * More test code refactoring * Made more modifications per review comments: - Logger switched to another type for WithLogger test cases - used %q for module name in error message - comments updated to be clearer - more test case cleanup * Added asserts to ensure module logger is not logging app start/stop events * Update module_test.go * Apply suggestions from code review Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

tchung1118 and others added 3 commits September 28, 2022 15:36

Adding more test cases for module-level logger

fdd16e1

More test case cleanups:

d26835c

- added another failure case for WithLogger in fx.Module - updated existing failure cases to expect any fx.Invokes to be not called

Merge branch 'master' into 894

43bba89

sywhang reviewed Sep 29, 2022

View reviewed changes

Addressing comments from PR review

1f525ac

- added comments on module.constructAllCustomLoggers - made module's parent logger the default logger for any given module - coding style changes per style guide - added more test cases for module-specific logger

sywhang reviewed Sep 29, 2022

View reviewed changes

More test code refactoring

fc72859

tchung1118 requested a review from sywhang September 29, 2022 22:35

sywhang reviewed Sep 30, 2022

View reviewed changes

Made more modifications per review comments:

94e06e3

- Logger switched to another type for WithLogger test cases - used %q for module name in error message - comments updated to be clearer - more test case cleanup

tchung1118 requested a review from sywhang September 30, 2022 14:51

sywhang reviewed Oct 3, 2022

View reviewed changes

Added asserts to ensure module logger is not logging app start/stop e…

432c2dd

…vents

tchung1118 requested a review from sywhang October 4, 2022 15:54

sywhang reviewed Oct 5, 2022

View reviewed changes

module_test.go Outdated Show resolved Hide resolved

Update module_test.go

d18ea0c

sywhang reviewed Oct 5, 2022

View reviewed changes

module_test.go Outdated Show resolved Hide resolved

module_test.go Outdated Show resolved Hide resolved

Apply suggestions from code review

2e4bbbb

sywhang approved these changes Oct 5, 2022

View reviewed changes

tchung1118 merged commit d073c7c into uber-go:master Oct 5, 2022

tchung1118 deleted the 894 branch October 5, 2022 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow fx.Module-specific custom loggers #947

Allow fx.Module-specific custom loggers #947

tchung1118 commented Sep 28, 2022

CLAassistant commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading

sywhang Sep 29, 2022

tchung1118 Sep 29, 2022

sywhang Sep 29, 2022

tchung1118 Sep 29, 2022

sywhang Sep 29, 2022

tchung1118 Sep 29, 2022

sywhang Sep 29, 2022

sywhang Sep 29, 2022

sywhang Sep 29, 2022

sywhang Sep 29, 2022

sywhang Sep 30, 2022

sywhang Sep 30, 2022

sywhang Sep 30, 2022

sywhang Sep 30, 2022

sywhang Sep 30, 2022

sywhang Sep 30, 2022

sywhang Oct 2, 2022

tchung1118 Oct 3, 2022

sywhang Oct 4, 2022

tchung1118 Oct 4, 2022

	buffer, ok := m.log.(*logBuffer)
	if ok {
	if buffer, ok := m.log.(*logBuffer); ok {


		require.NoError(t, app.Err())

		assert.Equal(t, []string{"Started", "Stopped"}, spy.EventTypes())

	return fmt.Errorf("fx.WithLogger(%v) from:\n%+v\nin Module: %v\nFailed: %w",
	return fmt.Errorf("fx.WithLogger(%v) from:\n%+v\nin Module: %q\nFailed: %w",

		// TODO: Use dig.FillProvideInfo to inspect the provided constructor
		// and fail the application if its signature didn't match.

Allow fx.Module-specific custom loggers #947

Allow fx.Module-specific custom loggers #947

Conversation

tchung1118 commented Sep 28, 2022

CLAassistant commented Sep 28, 2022 • edited Loading

codecov bot commented Sep 28, 2022 • edited Loading

Codecov Report

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

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

CLAassistant commented Sep 28, 2022 •

edited

Loading

codecov bot commented Sep 28, 2022 •

edited

Loading