Refactor tracing interceptor #450

XSAM · 2021-08-08T15:34:59Z

Split from #312. I will create the remaining parts of #312 once this PR gets merged.

codecov-commenter · 2021-08-08T15:36:58Z

Codecov Report

Merging #450 (7e04286) into v2 (6e2c2ac) will decrease coverage by 25.42%.
The diff coverage is 50.64%.

@@             Coverage Diff             @@
##               v2     #450       +/-   ##
===========================================
- Coverage   84.01%   58.59%   -25.43%     
===========================================
  Files          30       31        +1     
  Lines         932     1577      +645     
===========================================
+ Hits          783      924      +141     
- Misses        110      590      +480     
- Partials       39       63       +24

Impacted Files	Coverage Δ
chain.go	`0.00% <ø> (-90.91%)`	⬇️
interceptors/auth/auth.go	`100.00% <ø> (ø)`
interceptors/ratelimit/ratelimit.go	`60.00% <0.00%> (-40.00%)`	⬇️
interceptors/recovery/options.go	`78.57% <ø> (ø)`
metadata/single_key.go	`60.00% <ø> (ø)`
testing/testpb/interceptor_suite.go	`0.00% <0.00%> (ø)`
testing/testpb/test.manual_validator.pb.go	`0.00% <0.00%> (ø)`
util/backoffutils/backoff.go	`60.00% <ø> (ø)`
wrappers.go	`66.66% <ø> (-33.34%)`	⬇️
interceptors/reporter.go	`45.45% <20.00%> (-17.05%)`	⬇️
... and 25 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 72478fa...7e04286. Read the comment docs.

yashrsharma44 · 2021-08-19T20:02:39Z

Since #394 has been merged, there have been some changes in the structuring of tags(which are removed, replaced with logging.Fields) and tracing package has been removed.

Please rebase the changes.

yashrsharma44

Looks good to me, I will have another round of review, particularly the kv package, seems like a lot of chunk for tracing is present there :)

interceptors/tracing/interceptors.go

interceptors/tracing/reporter.go

bwplotka

Nice work! Did very quick pass with initial questions. Generally looks amazing! 💪🏽

bwplotka · 2021-08-26T11:12:28Z

interceptors/tracing/kv/value.go

+// instead of calling kv.Key(name).Bool(value) consider using a
+// convenience function provided by the api/key package -
+// key.Bool(name, value).
+func (k Key) Bool(v bool) KeyValue {


Not a fan of this package. Looks very low value to maintain another type system. Can we avoid it?

OTel uses a type-safe way to describe the type of attributes, I am afraid we will lose type information if we don't implement a type system.

interceptors/tracing/reporter.go

yashrsharma44

Everything looks good to me! Regarding the kv package, I don't have any solid suggestions, but do you think we can define the interface for the kv helper methods, and let the providers implement the kv implementation?

yashrsharma44 · 2021-08-31T07:16:37Z

interceptors/tracing/interceptors.go

+	SpanKindClient SpanKind = "client"
+)
+
+func reportable(tracer Tracer) interceptors.CommonReportableFunc {


This looks neat!

interceptors/tracing/interceptors_test.go

Co-authored-by: Yash Sharma <yashrsharma44@gmail.com>

XSAM · 2021-09-01T03:08:40Z

do you think we can define the interface for the kv helper methods, and let the providers implement the kv implementation.

Sounds good. @bwplotka WDYT?

bwplotka · 2022-04-06T15:01:29Z

Just doing pass on this, sorry for lag. Feel free to ping me on Go/CNCF slack next time. Do you wish to move forward with this @XSAM ? We need that (:

XSAM · 2022-04-06T15:05:57Z

yeah, i can still work on this.

bwplotka · 2022-04-06T15:15:06Z

Sweet, let me review this unless you want to change something

bwplotka

Nice, some comment first.

Good work 💪🏽

bwplotka · 2022-04-11T11:27:38Z

interceptors/tracing/interceptors_test.go

+	s.statusMessage = message
+}
+
+func (s *mockSpan) AddEvent(name string, attrs ...kv.KeyValue) {


Suggested change

func (s *mockSpan) AddEvent(name string, attrs ...kv.KeyValue) {

func (s *mockSpan) AddEvent(name string, attrs ...any) {

How bad would be to operate on interfaces (any) and expect implementation to deal with types?

NOTE: I know we could use generics, but in practice they have still some overhead, plus not everyone moved to Go 1.18, so we can add generic type of implementation later on (maybe even in non braking compatibility manner)

If we would move to interfaces, we won't be needing kv package - Otel implementation can then use "go.opentelemetry.io/otel/attribute"

…eptor

bwplotka

This looks amzing, LGTM. Just one comment. Do you have the OTLP side handy somewhere?

bwplotka · 2022-04-17T19:17:00Z

interceptors/tracing/tracing.go

+	// The default span status is OK, so it is not necessary to
+	// explicitly set an OK status on successful Spans unless it
+	// is to add an OK message or to override a previous status on the Span.
+	SetStatus(code codes.Code, msg string)


Amazing idea to use gRPC codes here, love it!

bwplotka · 2022-04-17T19:22:22Z

interceptors/tracing/interceptors_test.go

+	s.statusMessage = message
+}
+
+func (s *mockSpan) AddEvent(name string, attrs ...tracing.KeyValue) {


Idea: Remove kv and use keyvals ...any as in go-kit logger https://github.com/go-kit/kit/tree/master/log ? (:

well ..interface{}, since we can't just support Go 1.18 yet

Then we can use something like

// borrowed from https://github.com/go-logfmt/logfmt/blob/main/encode.go#L75 func kvToAttr(keyvals ...interface{}) []attribute.KeyValue { if len(keyvals) == 0 { return nil } if len(keyvals)%2 == 1 { keyvals = append(keyvals, nil) } j := 0 ret := make([]attribute.KeyValue, len(keyvals)/2) for i := 0; i < len(keyvals); i += 2 { k, ok := anyToString(keyvals[i]) if !ok { k = "<unsupported key type>" } v, ok := anyToString(keyvals[i+1]) if !ok { v = "<unsupported value type>" } ret[j] = attribute.String(k, v) j++ } return ret } func anyToString(value interface{}) (string, bool) { switch v := value.(type) { case nil: return "nil", true case string: return v, true case []byte: return string(v), true case error: return v.Error(), true case fmt.Stringer: return v.String(), true default: rvalue := reflect.ValueOf(value) switch rvalue.Kind() { case reflect.Array, reflect.Chan, reflect.Func, reflect.Map, reflect.Slice, reflect.Struct: return "", false case reflect.Ptr: if rvalue.IsNil() { return "nil", true } return anyToString(rvalue.Elem().Interface()) } return fmt.Sprint(v), true } }

bwplotka · 2022-04-21T13:52:55Z

Ping (:

bwplotka

Amazing, work thanks! Let's go!

XSAM force-pushed the refactor-tracing-interceptor branch from 03ffb9e to 8368b65 Compare August 8, 2021 15:55

yashrsharma44 added the v2 label Aug 13, 2021

yashrsharma44 added this to In progress in Milestone v2 via automation Aug 13, 2021

yashrsharma44 reviewed Aug 19, 2021

View reviewed changes

interceptors/tracing/interceptors.go Outdated Show resolved Hide resolved

interceptors/tracing/reporter.go Outdated Show resolved Hide resolved

interceptors/tracing/reporter.go Outdated Show resolved Hide resolved

XSAM force-pushed the refactor-tracing-interceptor branch from 8368b65 to 4a45a5b Compare August 25, 2021 08:27

XSAM added 3 commits August 25, 2021 16:49

Add tracing interceptor

7450973

Fix CI

be1e991

Follow dependencies changes

d0650ba

XSAM force-pushed the refactor-tracing-interceptor branch from 4a45a5b to d0650ba Compare August 25, 2021 09:26

Fix comments

822802a

bwplotka reviewed Aug 26, 2021

View reviewed changes

Avoid redundant else and return early

7854af0

yashrsharma44 approved these changes Aug 31, 2021

View reviewed changes

Update interceptors/tracing/interceptors_test.go

135fdbd

Co-authored-by: Yash Sharma <yashrsharma44@gmail.com>

bwplotka reviewed Apr 11, 2022

View reviewed changes

XSAM added 2 commits April 14, 2022 22:30

Remove kv package

9202a15

Merge remote-tracking branch 'origin/v2' into refactor-tracing-interc…

7e04286

…eptor

bwplotka approved these changes Apr 17, 2022

View reviewed changes

bwplotka reviewed Apr 17, 2022

View reviewed changes

Remove keyvalue

545d37a

bwplotka approved these changes Apr 25, 2022

View reviewed changes

bwplotka merged commit 83835dc into grpc-ecosystem:v2 Apr 25, 2022

Milestone v2 automation moved this from In progress to Done Apr 25, 2022

XSAM deleted the refactor-tracing-interceptor branch April 26, 2022 08:38

aimuz mentioned this pull request Mar 8, 2023

docs: add logging.InjectFields usage description #540

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor tracing interceptor #450

Refactor tracing interceptor #450

XSAM commented Aug 8, 2021 •

edited

codecov-commenter commented Aug 8, 2021 •

edited

yashrsharma44 commented Aug 19, 2021

yashrsharma44 left a comment

bwplotka left a comment

bwplotka Aug 26, 2021

XSAM Aug 31, 2021

yashrsharma44 left a comment

yashrsharma44 Aug 31, 2021

XSAM commented Sep 1, 2021

bwplotka commented Apr 6, 2022 •

edited

XSAM commented Apr 6, 2022

bwplotka commented Apr 6, 2022

bwplotka left a comment

bwplotka Apr 11, 2022 •

edited

bwplotka Apr 11, 2022

bwplotka left a comment

bwplotka Apr 17, 2022

bwplotka Apr 17, 2022

bwplotka Apr 17, 2022

bwplotka Apr 17, 2022

bwplotka commented Apr 21, 2022

bwplotka left a comment

	func (s *mockSpan) AddEvent(name string, attrs ...kv.KeyValue) {
	func (s *mockSpan) AddEvent(name string, attrs ...any) {

Refactor tracing interceptor #450

Refactor tracing interceptor #450

Conversation

XSAM commented Aug 8, 2021 • edited

codecov-commenter commented Aug 8, 2021 • edited

Codecov Report

yashrsharma44 commented Aug 19, 2021

yashrsharma44 left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashrsharma44 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XSAM commented Sep 1, 2021

bwplotka commented Apr 6, 2022 • edited

XSAM commented Apr 6, 2022

bwplotka commented Apr 6, 2022

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Apr 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka 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

bwplotka commented Apr 21, 2022

bwplotka left a comment

Choose a reason for hiding this comment

XSAM commented Aug 8, 2021 •

edited

codecov-commenter commented Aug 8, 2021 •

edited

bwplotka commented Apr 6, 2022 •

edited

bwplotka Apr 11, 2022 •

edited