Add Zap logger and access log interceptor #944

rinx · 2021-01-21T08:34:44Z

Description:

This PR is cherry-picked from #852 and revised.

Add Zap logger.
Revise logger interfaces to add Debugd, Infod, etc...
Add logger finalizer method Close()
Add AccessLogInterceptor

Related Issue:

nothing

How Has This Been Tested?:

nothing

Environment:

Go Version: 1.15.7
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.13.1

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

vdaas-ci · 2021-01-21T08:35:06Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - add changelog comment
🍱 /format - format codes and add licenses
✅ /gen-test - generate test codes
🏷️ /label - add labels
⏪ /rebase - rebase master
🔚 2️⃣ 🔚 /label actions/e2e-deploy - run E2E deploy & integration test

rinx · 2021-01-21T08:36:27Z

internal/log/glg/glg.go

@@ -81,6 +85,10 @@ func (l *logger) Infof(format string, vals ...interface{}) {
 	l.retry.Outf(l.glg.Infof, format, vals...)
 }

+func (l *logger) Infod(msg string, details ...interface{}) {
+	l.Infof(msg+"\tdetails: %#v", details)


Is this format okay? 🤔

please define default details format as const value on the top

codecov · 2021-01-21T08:42:31Z

Codecov Report

Merging #944 (98a97da) into master (5df9ec1) will increase coverage by 0.21%.
The diff coverage is 50.25%.

@@            Coverage Diff             @@
##           master     #944      +/-   ##
==========================================
+ Coverage   16.03%   16.25%   +0.21%     
==========================================
  Files         476      479       +3     
  Lines       23839    24016     +177     
==========================================
+ Hits         3822     3903      +81     
- Misses      19772    19867      +95     
- Partials      245      246       +1

Impacted Files	Coverage Δ
internal/log/format/format.go	`100.00% <ø> (ø)`
internal/log/glg/glg.go	`80.48% <0.00%> (-13.80%)`	⬇️
internal/log/logger/type.go	`100.00% <ø> (ø)`
internal/log/mock/logger.go	`62.50% <0.00%> (-37.50%)`	⬇️
...l/net/grpc/interceptor/server/logging/accesslog.go	`0.00% <0.00%> (ø)`
internal/log/log.go	`81.08% <50.00%> (-18.92%)`	⬇️
internal/log/zap/zap.go	`85.71% <85.71%> (ø)`
internal/log/zap/option.go	`100.00% <100.00%> (ø)`
internal/runner/runner.go	`88.52% <100.00%> (+0.19%)`	⬆️
internal/servers/server/option.go	`95.31% <100.00%> (+0.15%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5df9ec1...98a97da. Read the comment docs.

kpango

I never thought that you implement Infod like klog's InfoS, seems good idea.

kpango · 2021-01-22T03:42:12Z

internal/log/glg/glg.go

@@ -81,6 +85,10 @@ func (l *logger) Infof(format string, vals ...interface{}) {
 	l.retry.Outf(l.glg.Infof, format, vals...)
 }

+func (l *logger) Infod(msg string, details ...interface{}) {
+	l.Infof(msg+"\tdetails: %#v", details)


please define default details format as const value on the top

internal/log/zap/zap.go

internal/net/grpc/interceptor/server/logging/accesslog.go

internal/runner/runner.go

kpango · 2021-01-25T03:27:57Z

can you please check CI (test/internal & golangci-lint)

Signed-off-by: Rintaro Okamura <[email protected]> :white_check_mark: fix tests for logger type, format Signed-off-by: Rintaro Okamura <[email protected]>

Signed-off-by: Rintaro Okamura <[email protected]> :heavy_plus_sign: add zap logger Signed-off-by: Rintaro Okamura <[email protected]> :white_check_mark: add zap tests Signed-off-by: Rintaro Okamura <[email protected]> :white_check_mark: add zap tests Signed-off-by: Rintaro Okamura <[email protected]> :arrow_up: Upgrade zap and revise tests Signed-off-by: Rintaro Okamura <[email protected]>

Signed-off-by: Rintaro Okamura <[email protected]> :sparkles: Add Close method to logger implementations Signed-off-by: Rintaro Okamura <[email protected]>

Signed-off-by: Rintaro Okamura <[email protected]>

github-actions · 2021-01-25T04:54:24Z

internal/log/zap/zap.go

+	"github.com/vdaas/vald/internal/errors"
+	"github.com/vdaas/vald/internal/log/format"
+	"github.com/vdaas/vald/internal/log/level"
+


🚫 [golangci] _{reported by reviewdog 🐶}
File is not gci-ed (gci)

github-actions · 2021-01-25T04:54:24Z

internal/log/zap/option.go

+
+type Option func(l *logger)
+
+var (


🚫 [golangci] _{reported by reviewdog 🐶}
File is not gofumpt-ed (gofumpt)

github-actions · 2021-01-25T04:54:24Z

internal/log/zap/zap_test.go

+			if err := test.checkFunc(test.want, got, err); err != nil {
+				tt.Errorf("error = %v", err)
+			}
+


🚫 [golangci] _{reported by reviewdog 🐶}
File is not gofumpt-ed (gofumpt)

github-actions · 2021-01-25T04:54:24Z

internal/log/zap/zap_test.go

+			if err := test.checkFunc(test.want, l, err); err != nil {
+				tt.Errorf("error = %v", err)
+			}
+


🚫 [golangci] _{reported by reviewdog 🐶}
File is not gofumpt-ed (gofumpt)

github-actions · 2021-01-25T04:54:25Z

internal/runner/runner.go

@@ -76,6 +76,7 @@ func Do(ctx context.Context, opts ...Option) error {

 	if p.ShowVersion() {
 		log.Init(log.WithLevel(level.INFO.String()))
+		defer log.Close()


🚫 [golangci] _{reported by reviewdog 🐶}
Error return value of log.Close is not checked (errcheck)

github-actions · 2021-01-25T04:54:25Z

internal/runner/runner.go

@@ -95,6 +96,7 @@ func Do(ctx context.Context, opts ...Option) error {
 	} else {
 		log.Init()
 	}
+	defer log.Close()


🚫 [golangci] _{reported by reviewdog 🐶}
Error return value of log.Close is not checked (errcheck)

rinx · 2021-01-25T04:55:52Z

resolved.

kpango

LGTM

github-actions bot added area/helm area/internal team/set SET team size/XXL type/feature New feature labels Jan 21, 2021

rinx force-pushed the feature/internal/add-zap-and-access-log-interceptor branch from a333268 to dc740d1 Compare January 21, 2021 08:35

github-actions bot added the priority/medium label Jan 21, 2021

rinx commented Jan 21, 2021

View reviewed changes

rinx requested a review from kpango January 21, 2021 08:38

kpango requested changes Jan 22, 2021

View reviewed changes

rinx force-pushed the feature/internal/add-zap-and-access-log-interceptor branch 2 times, most recently from d268324 to fc1e535 Compare January 25, 2021 03:12

rinx added 9 commits January 25, 2021 12:32

🚸 Comment out unusable logtype and logformat

a1fed28

Signed-off-by: Rintaro Okamura <[email protected]> :white_check_mark: fix tests for logger type, format Signed-off-by: Rintaro Okamura <[email protected]>

✨ Add new methods to logger: Debugd, Infod, etc...

2feca26

Signed-off-by: Rintaro Okamura <[email protected]> :sparkles: Add Close method to logger implementations Signed-off-by: Rintaro Okamura <[email protected]>

✨ Add AccessLogInterceptor

1fec47b

Signed-off-by: Rintaro Okamura <[email protected]>

🔧 Add prometheus annotations for loki and tempo

65b5212

Signed-off-by: Rintaro Okamura <[email protected]>

🎨 Define detailsFormat as const

deebc9b

Signed-off-by: Rintaro Okamura <[email protected]>

🐛 Remove log.Close after Fatal level logs

88e63fe

Signed-off-by: Rintaro Okamura <[email protected]>

🔧 Add endTime field to access logs

9bb6023

Signed-off-by: Rintaro Okamura <[email protected]>

🔥 Remove unused import

98a97da

Signed-off-by: Rintaro Okamura <[email protected]>

rinx force-pushed the feature/internal/add-zap-and-access-log-interceptor branch from fc1e535 to 98a97da Compare January 25, 2021 04:48

github-actions bot reviewed Jan 25, 2021

View reviewed changes

kpango approved these changes Jan 25, 2021

View reviewed changes

kpango merged commit 93b1cee into master Jan 25, 2021

kpango deleted the feature/internal/add-zap-and-access-log-interceptor branch January 25, 2021 08:59

vdaas-ci mentioned this pull request Jan 29, 2021

Bug fix for gateway #963

Merged

18 tasks

kpango mentioned this pull request Feb 2, 2021

[major] v1.0.0 Release #974

Merged

11 tasks

rinx mentioned this pull request Feb 5, 2021

Add zap logger to chart schema #985

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Zap logger and access log interceptor #944

Add Zap logger and access log interceptor #944

rinx commented Jan 21, 2021

vdaas-ci commented Jan 21, 2021

rinx Jan 21, 2021

kpango Jan 22, 2021

rinx Jan 25, 2021

codecov bot commented Jan 21, 2021 •

edited

Loading

kpango left a comment

kpango Jan 22, 2021

kpango commented Jan 25, 2021

github-actions bot Jan 25, 2021

github-actions bot Jan 25, 2021

github-actions bot Jan 25, 2021

github-actions bot Jan 25, 2021

github-actions bot Jan 25, 2021

github-actions bot Jan 25, 2021

rinx commented Jan 25, 2021

kpango left a comment

Add Zap logger and access log interceptor #944

Add Zap logger and access log interceptor #944

Conversation

rinx commented Jan 21, 2021

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Jan 21, 2021

rinx Jan 21, 2021

Choose a reason for hiding this comment

kpango Jan 22, 2021

Choose a reason for hiding this comment

rinx Jan 25, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 21, 2021 • edited Loading

Codecov Report

kpango left a comment

Choose a reason for hiding this comment

kpango Jan 22, 2021

Choose a reason for hiding this comment

kpango commented Jan 25, 2021

github-actions bot Jan 25, 2021

Choose a reason for hiding this comment

github-actions bot Jan 25, 2021

Choose a reason for hiding this comment

github-actions bot Jan 25, 2021

Choose a reason for hiding this comment

github-actions bot Jan 25, 2021

Choose a reason for hiding this comment

github-actions bot Jan 25, 2021

Choose a reason for hiding this comment

github-actions bot Jan 25, 2021

Choose a reason for hiding this comment

rinx commented Jan 25, 2021

kpango left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 21, 2021 •

edited

Loading