Removed tags; Simplified interceptor code; Added logging fields editability. #394

bwplotka · 2021-01-30T22:44:47Z

Kind of biggy, but should be much cleaner.

The only surprise is that tracing is using logging.Fields to both fill span metadata (trace ID, sampled or not, span ID) as well. I think this is useful AND it was like that before 🤗

At least it's a clean separation of responsibilities who is using what vs generic tags.

Fixes #382

NOTE: I removed tracing package completely since in new PR we complete rewrite it anyway.

Signed-off-by: Bartlomiej Plotka [email protected]

bwplotka · 2021-01-30T22:46:02Z

bwplotka · 2021-01-30T22:49:44Z

No idea @yashrsharma44 how you PR passed linting... it fails now on this PR here 🤔

;p

go.sum

interceptors/auth/examples_test.go

interceptors/logging/doc.go

interceptors/logging/logging.go

johanbrandhorst · 2021-01-31T13:33:11Z

interceptors/logging/logging.go

+type FieldsIter interface {
+	Next() bool
+	At() (k, v string)
+}


What is this interface used for? Can't we just return a map[string]string? This seems needlessly complicated.

This is because kit logging way of specifying fields - we incorporated that but using our "safer" implementation of using fields encoded in []string

interceptors/logging/logging.go

interceptors/tracing/interceptors_test.go

interceptors/tracing/server.go

yashrsharma44 · 2021-02-01T12:57:26Z

No idea @yashrsharma44 how you PR passed linting... it fails now on this PR here 🤔

;p

I have added a PR - #395 to fix the same, let me know if anything else is required.

bwplotka · 2021-04-23T18:52:21Z

We need to get back to this (note to my self)

olivierlemasle · 2021-07-12T12:46:43Z

@bwplotka Do you still want to work on this?

bwplotka · 2021-07-12T16:30:51Z

Well I want, but I don't mind someone else taking this if they have time. 🤗 I am going for some vacation this week, and will be back in August, so won't touch it earlier then this ):

Help wanted! I will have time review/comment

bwplotka · 2021-08-06T06:13:02Z

on it!

bwplotka · 2021-08-06T07:25:16Z

PTAL @yashrsharma44 - addressed comments & rebased. Thanks for detailed review @johanbrandhorst !

…bility. Fixes #382 Signed-off-by: Bartlomiej Plotka <[email protected]> Removed tags; Simplified interceptor code; Added logging fields editability. Fixes #382 Signed-off-by: Bartlomiej Plotka <[email protected]> Fixed tests. Signed-off-by: Bartlomiej Plotka <[email protected]> Fixed open metrics test. Signed-off-by: Bartlomiej Plotka <[email protected]>

bwplotka · 2021-08-07T11:37:13Z

NOTE: I removed tracing package completely since in new PR (#312) we complete rewrite it anyway.

codecov-commenter · 2021-08-07T11:38:20Z

Codecov Report

Merging #394 (6157339) into v2 (6e2c2ac) will decrease coverage by 26.39%.
The diff coverage is 48.12%.

@@             Coverage Diff             @@
##               v2     #394       +/-   ##
===========================================
- Coverage   84.01%   57.61%   -26.40%     
===========================================
  Files          30       28        -2     
  Lines         932     1529      +597     
===========================================
+ Hits          783      881       +98     
- Misses        110      585      +475     
- Partials       39       63       +24

Impacted Files	Coverage Δ
chain.go	`0.00% <ø> (-90.91%)`	⬇️
interceptors/auth/auth.go	`100.00% <ø> (ø)`
interceptors/auth/metadata.go	`100.00% <ø> (ø)`
interceptors/ratelimit/ratelimit.go	`60.00% <0.00%> (-40.00%)`	⬇️
interceptors/recovery/options.go	`78.57% <ø> (ø)`
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% <ø> (ø)`
util/metautils/single_key.go	`60.00% <ø> (ø)`
wrappers.go	`66.66% <ø> (-33.34%)`	⬇️
... and 29 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 274df59...6157339. Read the comment docs.

bwplotka · 2021-08-07T11:39:19Z

PTAL @yashrsharma44

yashrsharma44

Some nitpicks/questions, otherwise
Looks awesome to me! 🥇

yashrsharma44 · 2021-08-13T15:31:15Z

interceptors/logging/logging.go

-type Fields []string
+// Iter returns FieldsIterator.
+func (f Fields) Iter() FieldsIterator {
+	// We start from -2 as we iterate over two items per iteration and first iteration will advance iterator to 0.


Should we raise an error in case the length of Fields array is not even? We always expect the key-value format, so we can raise an error. Thoughts?

interceptors/logging/doc.go

providers/kit/examples_test.go

interceptors/reporter.go

yashrsharma44 · 2021-08-13T15:49:31Z

interceptors/skip/interceptor_test.go

@@ -29,8 +28,13 @@ const (
 	keyMethod   = "skip.method"
 )

+var ctxKey = &ctxKeMarker{}
+
+type ctxKeMarker struct{}


Suggested change

type ctxKeMarker struct{}

type ctxKeyMarker struct{}

Is it intentional?

bwplotka · 2021-08-16T16:29:05Z

Thanks for comments Yash, will try to get them sorted soon

Signed-off-by: Bartlomiej Plotka <[email protected]>

bwplotka · 2021-08-17T16:46:37Z

Addressed 👍🏽 Thanks for review!

ti · 2021-08-27T07:16:37Z

why remove tags "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/tags"?

tags is not used jut for logging, it can used in multiple situations.

tags is really useful for grpc context, anything replace in interceptors/tags in grpc global tracing?

yashrsharma44 · 2021-08-27T07:55:57Z

Hi @ti !
We have replaced the tags interceptors with logging Fields. Since the use case is very simplistic, we didn't find the utility of maintaining yet another package for fields. Instead, you can use - logging.Fields, which support the same use case as tags, but with minimal code maintenance.
Here are some resources for reference -

These fields get injected into the context, so you can expect similar features as of tags, with minimal code :)

bwplotka requested review from johanbrandhorst and yashrsharma44 January 30, 2021 22:44

bwplotka force-pushed the clean branch from c7e0e85 to 07b4ff5 Compare January 30, 2021 22:49

johanbrandhorst reviewed Jan 31, 2021

View reviewed changes

bwplotka force-pushed the clean branch from 07b4ff5 to 4642377 Compare August 6, 2021 07:24

bwplotka force-pushed the clean branch 2 times, most recently from bd2685b to 2f12196 Compare August 6, 2021 12:51

bwplotka added the v2 label Aug 6, 2021

bwplotka force-pushed the clean branch 2 times, most recently from f3895ba to d2108e1 Compare August 7, 2021 09:57

bwplotka force-pushed the clean branch from d2108e1 to ff74a41 Compare August 7, 2021 11:36

bwplotka mentioned this pull request Aug 7, 2021

Refactor tracing implements #312

Closed

yashrsharma44 approved these changes Aug 13, 2021

View reviewed changes

yashrsharma44 mentioned this pull request Aug 13, 2021

make lint changes fixed #395

Closed

Addressed comments.

6157339

Signed-off-by: Bartlomiej Plotka <[email protected]>

bwplotka merged commit f8899ff into v2 Aug 17, 2021

bwplotka deleted the clean branch August 17, 2021 16:55

yashrsharma44 mentioned this pull request Aug 19, 2021

Refactor tracing interceptor #450

Merged

jpkrohling mentioned this pull request Dec 2, 2021

Remove opentracing from go.mod #477

Merged

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

Removed tags; Simplified interceptor code; Added logging fields editability. #394

Removed tags; Simplified interceptor code; Added logging fields editability. #394

bwplotka commented Jan 30, 2021 •

edited

Loading

bwplotka commented Jan 30, 2021

bwplotka commented Jan 30, 2021 •

edited

Loading

johanbrandhorst Jan 31, 2021

bwplotka Aug 6, 2021

yashrsharma44 commented Feb 1, 2021

bwplotka commented Apr 23, 2021

olivierlemasle commented Jul 12, 2021

bwplotka commented Jul 12, 2021

bwplotka commented Aug 6, 2021

bwplotka commented Aug 6, 2021

bwplotka commented Aug 7, 2021 •

edited

Loading

codecov-commenter commented Aug 7, 2021 •

edited

Loading

bwplotka commented Aug 7, 2021

yashrsharma44 left a comment •

edited

Loading

yashrsharma44 Aug 13, 2021

yashrsharma44 Aug 13, 2021

bwplotka commented Aug 16, 2021

bwplotka commented Aug 17, 2021

ti commented Aug 27, 2021 •

edited

Loading

yashrsharma44 commented Aug 27, 2021 •

edited

Loading

Removed tags; Simplified interceptor code; Added logging fields editability. #394

Removed tags; Simplified interceptor code; Added logging fields editability. #394

Conversation

bwplotka commented Jan 30, 2021 • edited Loading

bwplotka commented Jan 30, 2021

bwplotka commented Jan 30, 2021 • edited Loading

johanbrandhorst Jan 31, 2021

Choose a reason for hiding this comment

bwplotka Aug 6, 2021

Choose a reason for hiding this comment

yashrsharma44 commented Feb 1, 2021

bwplotka commented Apr 23, 2021

olivierlemasle commented Jul 12, 2021

bwplotka commented Jul 12, 2021

bwplotka commented Aug 6, 2021

bwplotka commented Aug 6, 2021

bwplotka commented Aug 7, 2021 • edited Loading

codecov-commenter commented Aug 7, 2021 • edited Loading

Codecov Report

bwplotka commented Aug 7, 2021

yashrsharma44 left a comment • edited Loading

Choose a reason for hiding this comment

yashrsharma44 Aug 13, 2021

Choose a reason for hiding this comment

yashrsharma44 Aug 13, 2021

Choose a reason for hiding this comment

bwplotka commented Aug 16, 2021

bwplotka commented Aug 17, 2021

ti commented Aug 27, 2021 • edited Loading

yashrsharma44 commented Aug 27, 2021 • edited Loading

bwplotka commented Jan 30, 2021 •

edited

Loading

bwplotka commented Jan 30, 2021 •

edited

Loading

bwplotka commented Aug 7, 2021 •

edited

Loading

codecov-commenter commented Aug 7, 2021 •

edited

Loading

yashrsharma44 left a comment •

edited

Loading

ti commented Aug 27, 2021 •

edited

Loading

yashrsharma44 commented Aug 27, 2021 •

edited

Loading