Add wrapped logger #119

andybradshaw · 2021-02-10T18:57:21Z

Before this PR

Currently, wrapped logging is not supported by the go logging libraries

After this PR

==COMMIT_MSG==
Add support for emitting wrapped.1 logs
==COMMIT_MSG==

Possible downsides?

In order to reduce code duplication (and potential bugs), we would have to either add the ToParams function to the typed logger interfaces, or export the DefaultLogger structs and do type assertions. I would imagine doing the later is preferable, since it doesn't really make sense to expose the params functions to the end user.

Wanted to put this up as a draft before expanding this to other log types, since it's a decent amount of code + tests to do if we don't think this is a good approach.

This change is

changelog-app · 2021-02-10T18:57:25Z

Generate changelog in `changelog/@unreleased`

Type

Description

Add support for emitting wrapped.1 logs.
Wrapped loggers expose the same interface to consumers as the logger of the underlying format.
This change includes a wrapped logging implementation for service.1 logs.

Check the box to generate changelog(s)

Generate changelog entry

andybradshaw · 2021-02-10T18:58:30Z

Tests are currently failing due to how the various logger impls handle having missing/empty fields. Looking at updating the matchers accordingly, but wanted to get this up for general feedback in the meantime

…evel

andybradshaw

Reviewable status: 0 of 21 files reviewed, 2 unresolved discussions

wlog/svclog/svc1log/logger_default.go, line 64 at r2 (raw file):

}

func ToParams(msg string, level wlog.Param, inParams []Param) []wlog.Param {

Turns out this doesn't need anything from the default logger... so we can just export the function without exporting the type. Still looking into making this internal, but at least it can be separated from struct.

wlog/wrappedlog/wrapped1log/context.go, line 28 at r2 (raw file):

// replace any logger that was previously set on the context (along with all parameters that may have been set on the
// logger).
func WithLogger(ctx context.Context, logger Logger) context.Context {

Pretty sure none of this is necessary, since a user will never actually store a wrapped logger, just the other loggers that the wrapped logger implements... will remove.

nmiyake

Thanks for the PR -- read through after our talk earlier today and have a much better understanding of the goal and the proposed implementation here.

Did you consider an alternate approach where each individual logger type supports instantiating a "wrapped" version? Was wondering that because, with the current approach, the "wrapped" logging package basically needs to have intimate knowledge of how each logger type it wraps operates and also sometimes requires the logger being wrapped to export functions to enable this.

Having the wrapped logger be its own package would make more sense if it could generically wrap any type of logger, but if we have to custom-implement the wrapped logger per logger type anyway then I'm not sure that it actually provides an advantage. In the alternate approach, the wrapped1log would basically just export the constants for things like the type value, payload etc.

May make sense to schedule another time for us to sync up now that I feel more familiar with the problem space and the proposed implementation.

nmiyake · 2021-02-25T06:05:20Z

wlog/diaglog/diag1log/logger_default.go

 	}
 	return outParams
 }

-var defaultTypeParam = []wlog.Param{
+var defaultParams = []wlog.Param{


Is there a reason that this is being renamed?

nmiyake · 2021-02-25T06:06:16Z

wlog/svclog/svc1log/context.go

@@ -51,7 +51,7 @@ func WithLoggerParams(ctx context.Context, params ...Param) context.Context {
 }

 // FromContext returns the Logger stored in the provided context. If no logger is set on the context, returns the logger
-// created by calling DefaultLogger. If the context contains a TraceID set using wtracing, the returned logger has that
+// created by calling defaultLogger. If the context contains a TraceID set using wtracing, the returned logger has that


Extraneous edit caused by refactor?

nmiyake · 2021-02-25T06:09:58Z

wlog/svclog/svc1log/logger_default.go

 	"github.com/palantir/witchcraft-go-logging/wlog"
 )

+var DebugLevelParam = wlog.NewParam(func(entry wlog.LogEntry) {


What's the high-level motivation/explanation for why the logger level is now a param?

If we were to actually do this, then these should probably be functions that return the param -- it's not good practice to have exported package-level variables since they are mutable (anyone importing the package could do something like svc1log.DebugLevelParam = nil and reassign the global value)

nmiyake · 2021-02-25T06:11:05Z

wlog/wrappedlog/wrapped1log/context.go

@@ -0,0 +1,31 @@
+// Copyright (c) 2018 Palantir Technologies. All rights reserved.


nit: since this is a new file, copyright should be 2021 (I'm guessing this was just copy-pasted -- if you omit the header entirely, then the "verify" task will add the proper header with the current year as part of the "license" task)

nmiyake · 2021-02-25T06:12:54Z

wlog/wrappedlog/wrapped1log/context.go

+// replace any logger that was previously set on the context (along with all parameters that may have been set on the
+// logger).
+func WithLogger(ctx context.Context, logger Logger) context.Context {
+	// Set _all_ the underlying logger types


What does this comment mean? Does this code do something beyond just replace the logger on the specific context?

Never mind, see updated comment that this will not be needed

Is this still needed? Per my last comment, thought you had a previous comment that stated that this wouldn't need to be set here

nmiyake · 2021-02-25T06:33:50Z

wlog/logger_provider_jsonmarshal.go

 	}
 }

 func (l *jsonMapLogger) Info(msg string, params ...Param) {
 	switch l.level {
 	case DebugLevel, InfoLevel:
-		l.logOutput(append(params, StringParam("message", msg), StringParam("level", "INFO")))
+		l.logOutput(append([]Param{StringParam("message", msg), StringParam("level", "INFO")}, params...))


Why does this logic for "Info" not match the others?

Fixed, I think this is the correct logic

nmiyake

Thanks for updating! Took a deeper look.

On the whole, am on board with this change. There are some minor points of feedback that I left as part of the review.

At a macro level, would request that the change that makes the logger provider API more generic (removes TimeKey and LevelKey etc. from the generic logger providers) be made as a separate PR -- this is a behavioral change to publicly exported API, so would like to have it be reviewed and documented separately.

nmiyake · 2021-03-08T04:36:03Z

wlog/diaglog/diag1log/logger_default.go

 	}
 	return outParams
 }

-var defaultTypeParam = []wlog.Param{
+var defaultTypeParams = []wlog.Param{


Name should be kept as defaultTypeParam -- there's still only one param entry in the slice, and don't think that the change is significant enough to rename and introduce the new diff.

nmiyake · 2021-03-08T04:38:32Z

wlog/logger_provider_jsonmarshal.go

@@ -41,28 +41,28 @@ func (l *jsonMapLogger) Log(params ...Param) {
 func (l *jsonMapLogger) Debug(msg string, params ...Param) {
 	switch l.level {
 	case DebugLevel:
-		l.logOutput(append(params, StringParam("message", msg), StringParam("level", "DEBUG")))
+		l.logOutput(append([]Param{StringParam("message", msg), StringParam("level", "DEBUG")}, params...))


Why are params at the end now rather than the beginning? Not a big deal as long as everything works, but would like to understand if this is intentional/necessary and, if so, why it works.

Yeah, this is intentional so that the svc1logger can overwrite the message field using a param, want its params to be applied after the ones defined here.

nmiyake · 2021-03-08T04:41:52Z

wlog/svclog/svc1log/context.go

@@ -84,7 +84,7 @@ func safeAndUnsafeParamsFromParams(params []Param) (safe map[string]interface{},
 }

 // loggerFromContext returns the logger stored in the provided context. If no logger is set on the context, returns the
-// logger created by calling DefaultLogger.
+// logger created by calling defaultLogger.


nit: should be defaultLoggerCreator.

Searching through the codebase, it looks like this typo/incorrect attribution happens for most of the logger types. Given that this PR doesn't modify this function, I would prefer that this not be corrected in this PR and we open a separate PR that fixes this documentation issue for all the relevant functions.

nmiyake · 2021-03-08T04:45:37Z

wlog/svclog/svc1log/logger_default.go

 	"github.com/palantir/witchcraft-go-logging/wlog"
 )

+var debugLevelParam = wlog.NewParam(func(entry wlog.LogEntry) {


Is the declaration of these variables as top-level variables rather than inlining as part of the function return intentional for purposes of performance/allocation? If so, then that's fine, but would add comment that explains this and then declare these in block form:

var ( debugLevelParam = ... infoLevelParam = ... // etc. )

If there isn't a performance-related or otherwise intentional reason for these declarations, then would prefer that they not be declared as variables and inline in the functions instead.

Yeah, this is so they are initialized once. Will update.

nmiyake · 2021-03-08T04:46:23Z

wlog/svclog/svc1log/logger_default.go

 	}
 	return outParams
 }

-var defaultTypeParam = []wlog.Param{
+var defaultParams = []wlog.Param{


Do not rename variable

nmiyake · 2021-03-08T04:47:29Z

wlog-zap/internal/provider.go

@@ -33,7 +32,6 @@ type loggerProvider struct{}

 func (lp *loggerProvider) NewLogger(w io.Writer) wlog.Logger {
 	logger, atomicLevel := newZapLogger(w, wlog.InfoLevel, zapcore.EncoderConfig{
-		TimeKey:        wlog.TimeKey,


Would prefer that this refactor be done as a separate commit that doesn't add wrapped logger

Done, opened https://github.com/palantir/witchcraft-go-logging/pull/123/files

commit b33a0cf Author: akrishna <akrishna@palantir.com> Date: Wed Mar 10 12:19:33 2021 -0500 CR commit 17133f4 Author: akrishna <akrishna@palantir.com> Date: Tue Mar 9 13:09:15 2021 -0500 add comment commit 925fc37 Author: akrishna <akrishna@palantir.com> Date: Tue Mar 9 17:50:24 2021 +0000 Add generated changelog entries commit f19fe83 Author: akrishna <akrishna@palantir.com> Date: Tue Mar 9 12:50:24 2021 -0500 remove time, level, and message keys in logger provider commit a1d5ca7 Author: Excavator Bot <33266368+svc-excavator-bot@users.noreply.github.com> Date: Thu Mar 4 09:51:25 2021 -0800 Excavator: Render CircleCI file using template specified in .circleci/template.sh (#122) commit 246e74c Author: Tony Abboud <tdabboud@hotmail.com> Date: Tue Feb 16 18:25:05 2021 +0000 Autorelease 1.10.0 commit 76beace Author: smoorpal <56262229+smoorpal@users.noreply.github.com> Date: Tue Feb 16 13:05:05 2021 -0500 improvement: add tags to span (#120) Write trace span tags

…o-logging into ab/wrapped-logging

nmiyake

Looks pretty good overall! Some feedback.

nmiyake · 2021-03-12T18:46:52Z

wlog/wrappedlog/wrapped1log/context.go

+// replace any logger that was previously set on the context (along with all parameters that may have been set on the
+// logger).
+func WithLogger(ctx context.Context, logger Logger) context.Context {
+	// Set _all_ the underlying logger types


Is this still needed? Per my last comment, thought you had a previous comment that stated that this wouldn't need to be set here

nmiyake · 2021-03-12T18:52:06Z

wlog/wrappedlog/wrapped1log/logger.go

+	Service(params ...svc1log.Param) svc1log.Logger
+}
+
+func New(w io.Writer, level wlog.LogLevel, name string, version string) Logger {


nit: combine declaration for last two parameters:

name, version string)

nmiyake · 2021-03-12T18:52:12Z

wlog/wrappedlog/wrapped1log/logger.go

+	return NewFromProvider(w, level, wlog.DefaultLoggerProvider(), name, version)
+}
+
+func NewFromProvider(w io.Writer, level wlog.LogLevel, creator wlog.LoggerProvider, name string, version string) Logger {


Same as above

nmiyake · 2021-03-12T18:53:06Z

wlog/wrappedlog/wrapped1log/logger_service.go

+	l.logger.SetLevel(level)
+}
+
+func (l *wrappedSvc1Logger) ToServiceParams(message string, levelParam wlog.Param, inParams []svc1log.Param) []wlog.Param {


Does this function need to be exported? If not, would keep internal.

…o-logging into ab/wrapped-logging

nmiyake · 2021-03-12T19:22:44Z

changelog/@unreleased/pr-119.v2.yml

@@ -0,0 +1,8 @@
+type: improvement


Can you make this (and type field below) "feature"

nmiyake · 2021-03-12T19:27:04Z

wlog/wrappedlog/wrapped1log/logger_service.go

@@ -0,0 +1,56 @@
+// Copyright (c) 2018 Palantir Technologies. All rights reserved.


Oops this should be 2021

nmiyake

Thanks!

Add wrapped logger

aa7f142

andybradshaw added 2 commits February 24, 2021 11:48

Update zap and zerolog to account for params no longer being at top l…

853bbfa

…evel

Don't expose service default logger

626e459

andybradshaw commented Feb 24, 2021

View reviewed changes

nmiyake reviewed Feb 25, 2021

View reviewed changes

CR

f9c0039

nmiyake requested changes Mar 8, 2021

View reviewed changes

advayakrishna mentioned this pull request Mar 9, 2021

Remove time, level, and message keys in logger provider #123

Merged

akrishna added 8 commits March 9, 2021 13:13

CR

d3b8796

Merge remote-tracking branch 'origin/develop' into ab/wrapped-logging

97238a1

Add generated changelog entries

4e652f4

add changelog

6bbb10c

Add generated changelog entries

d058dc8

Merge branch 'ab/wrapped-logging' of github.com:palantir/witchcraft-g…

9724721

…o-logging into ab/wrapped-logging

Add generated changelog entries

4f2348d

advayakrishna closed this Mar 12, 2021

advayakrishna reopened this Mar 12, 2021

nmiyake requested changes Mar 12, 2021

View reviewed changes

akrishna added 2 commits March 12, 2021 14:18

CR

2d7a4cb

Merge branch 'ab/wrapped-logging' of github.com:palantir/witchcraft-g…

bc2d1b3

…o-logging into ab/wrapped-logging

nmiyake requested changes Mar 12, 2021

View reviewed changes

CR

1483e80

nmiyake reviewed Mar 12, 2021

View reviewed changes

fix copyright

662cb48

nmiyake approved these changes Mar 12, 2021

View reviewed changes

nmiyake merged commit 9732c8a into develop Mar 12, 2021

nmiyake deleted the ab/wrapped-logging branch March 12, 2021 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wrapped logger #119

Add wrapped logger #119

andybradshaw commented Feb 10, 2021 •

edited by bmoylan

Loading

changelog-app bot commented Feb 10, 2021 •

edited by advayakrishna

Loading

andybradshaw commented Feb 10, 2021

andybradshaw left a comment

nmiyake left a comment

nmiyake Feb 25, 2021

nmiyake Feb 25, 2021

nmiyake Feb 25, 2021

nmiyake Feb 25, 2021

nmiyake Feb 25, 2021

nmiyake Feb 25, 2021

nmiyake Mar 12, 2021

nmiyake Feb 25, 2021

advayakrishna Mar 5, 2021

nmiyake left a comment

nmiyake Mar 8, 2021

nmiyake Mar 8, 2021

advayakrishna Mar 9, 2021

nmiyake Mar 8, 2021

nmiyake Mar 8, 2021

advayakrishna Mar 9, 2021

nmiyake Mar 8, 2021

nmiyake Mar 8, 2021

advayakrishna Mar 9, 2021

nmiyake left a comment

nmiyake Mar 12, 2021

nmiyake Mar 12, 2021

nmiyake Mar 12, 2021

nmiyake Mar 12, 2021

nmiyake Mar 12, 2021

nmiyake Mar 12, 2021

nmiyake left a comment

		@@ -0,0 +1,31 @@
		// Copyright (c) 2018 Palantir Technologies. All rights reserved.

		@@ -0,0 +1,56 @@
		// Copyright (c) 2018 Palantir Technologies. All rights reserved.

Add wrapped logger #119

Add wrapped logger #119

Conversation

andybradshaw commented Feb 10, 2021 • edited by bmoylan Loading

Before this PR

After this PR

Possible downsides?

changelog-app bot commented Feb 10, 2021 • edited by advayakrishna Loading

Generate changelog in changelog/@unreleased

andybradshaw commented Feb 10, 2021

andybradshaw left a comment

Choose a reason for hiding this comment

nmiyake 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

nmiyake 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

nmiyake 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

nmiyake left a comment

Choose a reason for hiding this comment

andybradshaw commented Feb 10, 2021 •

edited by bmoylan

Loading

changelog-app bot commented Feb 10, 2021 •

edited by advayakrishna

Loading

Generate changelog in `changelog/@unreleased`