Add a gRPC interceptor for embedding payloads into trace spans #900

rinx · 2020-12-23T07:13:12Z

Signed-off-by: Rintaro Okamura rintaro.okamura@gmail.com

Description:

In this PR, I added a gRPC interceptor for embedding gRPC request & response payloads into trace spans.

Related Issue:

nothing

How Has This Been Tested?:

nothing

Environment:

Go Version: 1.15.2
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.12.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 · 2020-12-23T07:13:28Z

[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

internal/net/grpc/interceptor/server/trace/payload.go

codecov · 2020-12-23T07:19:57Z

Codecov Report

Merging #900 (b1af006) into master (fa1db2b) will increase coverage by 0.04%.
The diff coverage is 38.23%.

@@            Coverage Diff             @@
##           master     #900      +/-   ##
==========================================
+ Coverage   15.94%   15.98%   +0.04%     
==========================================
  Files         474      474              
  Lines       23693    23707      +14     
==========================================
+ Hits         3778     3790      +12     
- Misses      19678    19679       +1     
- Partials      237      238       +1

Impacted Files	Coverage Δ
...nal/net/grpc/interceptor/server/recover/recover.go	`0.00% <0.00%> (ø)`
pkg/agent/core/ngt/usecase/agentd.go	`0.00% <ø> (ø)`
...ent/sidecar/usecase/initcontainer/initcontainer.go	`0.00% <ø> (ø)`
pkg/agent/sidecar/usecase/sidecar/sidecar.go	`0.00% <ø> (ø)`
pkg/discoverer/k8s/usecase/discovered.go	`0.00% <ø> (ø)`
pkg/gateway/vald/usecase/vald.go	`0.00% <ø> (ø)`
pkg/manager/backup/cassandra/usecase/backupd.go	`0.00% <ø> (ø)`
pkg/manager/backup/mysql/usecase/backupd.go	`0.00% <ø> (ø)`
pkg/manager/compressor/usecase/compressord.go	`0.00% <ø> (ø)`
pkg/manager/index/usecase/indexer.go	`0.00% <ø> (ø)`
... and 9 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 fa1db2b...4f4924a. Read the comment docs.

kpango

Others LGTM

internal/net/grpc/interceptor/server/trace/payload.go

kpango · 2021-01-20T11:37:20Z

In this PR, the package structure of the interceptor seems to have changed.
Could you please move internal/net/grpc/interceptor.go to internal/net/grpc/interceptor/server/recover/recover_interceptor.go?
and remove each configuration point for RecoverInterceptor configuration from the use case layer of each pkg so that it can be set from the WithGRPCInterceptors function as well as TraceInterceptor.
It would also be helpful if the RecoverInterceptor could be set to be on by default in helm.

rinx · 2021-01-21T00:59:57Z

Thanks. that's good idea. I'll try to do it.

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com> fixup! ✨ Add TracePayloadInterceptor Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com> 📄 update license headers Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com> ✏️ Fix typo Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

rinx · 2021-01-21T02:34:03Z

@kpango this PR is revised as you mentioned. could you please review again?

github-actions · 2021-01-21T02:37:26Z

internal/net/grpc/interceptor/server/trace/payload.go

+	traceAttrGRPCResponsePayload = "grpc.response.payload"
+)
+
+var (


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

github-actions · 2021-01-21T02:37:26Z

internal/net/grpc/interceptor/server/trace/payload.go

+)
+
+var (
+	bufferPool = sync.Pool{


🚫 [golangci] _{reported by reviewdog 🐶}
bufferPool is a global variable (gochecknoglobals)

github-actions · 2021-01-21T02:37:26Z

internal/net/grpc/interceptor/server/recover/recover_test.go

@@ -84,23 +85,24 @@ func TestRecoverInterceptor(t *testing.T) {
 			if err := test.checkFunc(test.want, got); err != nil {
 				tt.Errorf("error = %v", err)
 			}
+


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

github-actions · 2021-01-21T02:37:26Z

internal/net/grpc/interceptor/server/recover/recover_test.go

@@ -147,6 +149,7 @@ func TestRecoverStreamInterceptor(t *testing.T) {
 			if err := test.checkFunc(test.want, got); err != nil {
 				tt.Errorf("error = %v", err)
 			}
+


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

github-actions · 2021-01-21T02:37:26Z

internal/net/grpc/interceptor/server/recover/recover.go

+//
+
+// Package recover provides gRPC interceptors for recovery
+package recover


🚫 [golangci] _{reported by reviewdog 🐶}
package name recover has same name as predeclared identifier (predeclared)

github-actions · 2021-01-21T02:37:26Z

internal/net/grpc/interceptor/server/recover/recover_test.go

-// Package grpc provides generic functionality for grpc
-package grpc
+// Package recover provides gRPC interceptors for recovery
+package recover


🚫 [golangci] _{reported by reviewdog 🐶}
package name recover has same name as predeclared identifier (predeclared)

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

kpango

Looks Really Good for Me, thanks for revise.

rinx · 2021-01-21T03:33:10Z

Thanks!

github-actions bot added area/helm area/internal size/XXXL type/feature New feature priority/medium labels Dec 23, 2020

github-actions bot reviewed Dec 23, 2020

View reviewed changes

internal/net/grpc/interceptor/server/trace/payload.go Outdated Show resolved Hide resolved

github-actions bot added the team/set SET team label Dec 23, 2020

rinx force-pushed the feature/internal/add-rpc-payload-grpc-interceptor branch from 05ed181 to 6e87b03 Compare December 23, 2020 07:38

rinx force-pushed the feature/internal/add-access-log-grpc-interceptor branch from 80384c2 to 5adf052 Compare December 24, 2020 02:36

rinx force-pushed the feature/internal/add-rpc-payload-grpc-interceptor branch from 6e87b03 to aaa7b7a Compare December 24, 2020 02:44

rinx force-pushed the feature/internal/add-access-log-grpc-interceptor branch from 5adf052 to af018a2 Compare January 6, 2021 01:52

rinx force-pushed the feature/internal/add-rpc-payload-grpc-interceptor branch from aaa7b7a to 8f7b0ff Compare January 6, 2021 02:05

rinx force-pushed the feature/internal/add-access-log-grpc-interceptor branch from af018a2 to f5e2132 Compare January 12, 2021 01:18

rinx force-pushed the feature/internal/add-rpc-payload-grpc-interceptor branch from 8f7b0ff to c80b8d0 Compare January 12, 2021 01:19

rinx force-pushed the feature/internal/add-access-log-grpc-interceptor branch from f5e2132 to f699079 Compare January 12, 2021 02:19

rinx force-pushed the feature/internal/add-access-log-grpc-interceptor branch 2 times, most recently from ad99d8b to df36f12 Compare January 20, 2021 06:31

rinx force-pushed the feature/internal/add-rpc-payload-grpc-interceptor branch from c80b8d0 to 6f75ff2 Compare January 20, 2021 06:41

rinx force-pushed the feature/internal/add-access-log-grpc-interceptor branch from df36f12 to 3e558e5 Compare January 20, 2021 07:25

rinx force-pushed the feature/internal/add-rpc-payload-grpc-interceptor branch from 6f75ff2 to 2297533 Compare January 20, 2021 09:24

rinx changed the base branch from feature/internal/add-access-log-grpc-interceptor to master January 20, 2021 09:25

github-actions bot added size/L and removed size/L labels Jan 20, 2021

rinx force-pushed the feature/internal/add-rpc-payload-grpc-interceptor branch from 7e8cb6b to dbbaa13 Compare January 20, 2021 09:45

github-actions bot added size/L and removed size/L labels Jan 20, 2021

rinx added the size/L label Jan 20, 2021

kpango requested changes Jan 20, 2021

View reviewed changes

internal/net/grpc/interceptor/server/trace/payload.go Outdated Show resolved Hide resolved

internal/net/grpc/interceptor/server/trace/payload.go Outdated Show resolved Hide resolved

rinx added 6 commits January 21, 2021 11:31

✅ Add test cases for WithGRPCInterceptors

519d2b9

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

♻️ Use consts for trace attribute names

42f019b

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

♻️ Use sync.pool as buffer pool for JSON marshaler

0ca142d

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

♻️ Use internal/encoding/json for marshaler

a7ee29a

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

♻️ Split RecoverInterceptor as optional interceptor

4e681d8

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

rinx force-pushed the feature/internal/add-rpc-payload-grpc-interceptor branch from c164b20 to 4e681d8 Compare January 21, 2021 02:31

github-actions bot added area/agent/core area/agent/sidecar area/discoverer area/gateway area/manager/backup area/manager/index area/manager/replication area/meta labels Jan 21, 2021

github-actions bot reviewed Jan 21, 2021

View reviewed changes

🎨 Revise format of interceptor field

4f4924a

Signed-off-by: Rintaro Okamura <rintaro.okamura@gmail.com>

rinx requested a review from kpango January 21, 2021 02:53

rinx added the actions/e2e-deploy label Jan 21, 2021

kpango approved these changes Jan 21, 2021

View reviewed changes

kpango merged commit efad135 into master Jan 21, 2021

kpango deleted the feature/internal/add-rpc-payload-grpc-interceptor branch January 21, 2021 03:29

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a gRPC interceptor for embedding payloads into trace spans #900

Add a gRPC interceptor for embedding payloads into trace spans #900

rinx commented Dec 23, 2020 •

edited

Loading

vdaas-ci commented Dec 23, 2020

codecov bot commented Dec 23, 2020 •

edited

Loading

kpango left a comment

kpango commented Jan 20, 2021

rinx commented Jan 21, 2021

rinx commented Jan 21, 2021

github-actions bot Jan 21, 2021

github-actions bot Jan 21, 2021

github-actions bot Jan 21, 2021

github-actions bot Jan 21, 2021

github-actions bot Jan 21, 2021

github-actions bot Jan 21, 2021

kpango left a comment

rinx commented Jan 21, 2021

Add a gRPC interceptor for embedding payloads into trace spans #900

Add a gRPC interceptor for embedding payloads into trace spans #900

Conversation

rinx commented Dec 23, 2020 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Dec 23, 2020

codecov bot commented Dec 23, 2020 • edited Loading

Codecov Report

kpango left a comment

Choose a reason for hiding this comment

kpango commented Jan 20, 2021

rinx commented Jan 21, 2021

rinx commented Jan 21, 2021

github-actions bot Jan 21, 2021

Choose a reason for hiding this comment

github-actions bot Jan 21, 2021

Choose a reason for hiding this comment

github-actions bot Jan 21, 2021

Choose a reason for hiding this comment

github-actions bot Jan 21, 2021

Choose a reason for hiding this comment

github-actions bot Jan 21, 2021

Choose a reason for hiding this comment

github-actions bot Jan 21, 2021

Choose a reason for hiding this comment

kpango left a comment

Choose a reason for hiding this comment

rinx commented Jan 21, 2021

rinx commented Dec 23, 2020 •

edited

Loading

codecov bot commented Dec 23, 2020 •

edited

Loading