Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logger Interface #1775

Merged
merged 1 commit into from Mar 14, 2019

Conversation

4 participants
@pjdufour-truss
Copy link
Contributor

pjdufour-truss commented Feb 21, 2019

Description

This PR migrates our use of zap.Logger type to an internal logger interface for each package. This new approach provides something similar to a contract that explicitly defines each packages logging
requirements. This approach takes advantage of Go's feature that doesn't require explicit implements or extends of interfaces. This feature allows us to use interfaces internal to a package that simplify our inter-dependencies, though they do require defining our logger interface more than once for each package. This adds some marginal cost, but is much more scalable for a growing codebase.

Additionally, since we are now using an interface, we can make project-specific enhancements to our loggers that may not be suitable for merging upstream with zap.

Functionally, this PR allows us to pass the git_branch and git_commit fields through the request cycle and will provide hooks to add additional logging fields over time. For example, we'd like to log task id, session id, and a unique request id with every relevant log message so we can more easily search our application logs.

Reviewer Notes

  • Should we make the logger interface for each package public? While not "required", private interfaces aren't shown by godoc.
  • How do we document this pattern?

Setup

None

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using bin/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in IE.
  • Any new client dependencies (Google Analytics, hosted libraries, CDNs, etc) have been:
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

Screenshots

N.A.

@pjdufour-truss pjdufour-truss changed the title WIP Logger Interface [WIP] Logger Interface Feb 21, 2019

@@ -21,7 +20,7 @@ import (

type UploaderSuite struct {
testingsuite.PopTestSuite
logger *zap.Logger
logger logint.Logger

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Feb 21, 2019

Contributor

What is logint?

@pjdufour-truss pjdufour-truss requested review from lynzt , ntwyman and sarboc Feb 21, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 21, 2019

Codecov Report

Merging #1775 into master will increase coverage by <.01%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #1775      +/-   ##
==========================================
+ Coverage   49.58%   49.58%   +<.01%     
==========================================
  Files         429      430       +1     
  Lines       18493    18497       +4     
  Branches     1632     1632              
==========================================
+ Hits         9170     9172       +2     
- Misses       8519     8521       +2     
  Partials      804      804
zapLogger, err := logging.Config(env, v.GetBool("debug-logging"))
if err != nil {
log.Fatalf("Failed to initialize Zap logging due to %v", err)
}
zap.ReplaceGlobals(zapLogger)

logger := &webserverLogger{zapLogger}
if len(gitBranch) > 0 || len(gitCommit) > 0 {

This comment has been minimized.

@mikena-truss

mikena-truss Feb 22, 2019

Contributor

Can this entire block can be:

        fields := make([]zap.Field, 0)
		if len(gitBranch) > 0 {
			fields = append(fields, zap.String("git_branch", gitBranch))
		}
		if len(gitCommit) > 0 {
			fields = append(fields, zap.String("git_commit", gitCommit))
		}
		logger = zapLogger.With(fields...)

You'll automatically make the slice, but With handles that just fine and it saves some readability and len calls

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Feb 22, 2019

Author Contributor

Yeah, looks like With just returns the object pointer without copying. Good idea.

https://github.com/uber-go/zap/blob/master/logger.go#L159

}

// WithOptions wraps zap.WithOptions
func withOptions(logger logger, opts ...zap.Option) logger {

This comment has been minimized.

@mikena-truss

mikena-truss Feb 22, 2019

Contributor

Why wrap this instead of adding it to the interface?

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Feb 22, 2019

Author Contributor

👍

)

type logger interface {
Debug(msg string, fields ...zap.Field)

This comment has been minimized.

@rdhariwal

rdhariwal Feb 26, 2019

Contributor

Would we consider simplifying the signature for debug/error etc functions one more step? We can use a string map like this Debug(msg string, arguments map[string] string) and that should remove zap library dependency. I realize that might have a chain reaction effect of changes in all handlers where logger is already using zap but that should remove one more dependency for us.

This comment has been minimized.

@rdhariwal

rdhariwal Feb 26, 2019

Contributor

on the other hand I do see how zap can handle not just string but other types.. so take this comment with a grain of salt

This comment has been minimized.

@pjdufour-truss

pjdufour-truss Feb 26, 2019

Author Contributor

You're in my mind. Personally, I think that's a good objective, but want to pave the road there first.

@rdhariwal

This comment has been minimized.

Copy link
Contributor

rdhariwal commented Feb 26, 2019

Looks great to me overall, great job! I would like to add my 2 cents to what @mikena-truss has already mentioned about having a single interface across packages for the logger so that developers don't have to translate which "version" of the logger they are working with and DI will work very cleanly if there is just once interface.

Having said that you can change me mind :)

@pjdufour-truss pjdufour-truss force-pushed the logger_interface branch 5 times, most recently from 732e799 to fd65860 Feb 26, 2019

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

🚀

@pjdufour-truss pjdufour-truss force-pushed the logger_interface branch 9 times, most recently from fe45ce6 to 8fc6920 Mar 12, 2019

@pjdufour-truss pjdufour-truss force-pushed the logger_interface branch from 8fc6920 to 44536be Mar 14, 2019

@pjdufour-truss pjdufour-truss changed the title [WIP] Logger Interface Logger Interface Mar 14, 2019

@pjdufour-truss pjdufour-truss merged commit 379b766 into master Mar 14, 2019

19 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch 60% of diff hit (target 49.58%)
Details
codecov/project 49.58% (+<.01%) compared to e4730c1
Details

@pjdufour-truss pjdufour-truss deleted the logger_interface branch Mar 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.