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

v2 tags: Add tests and examples replacement for gogo.moretags #382

Closed
bwplotka opened this issue Jan 16, 2021 · 12 comments
Closed

v2 tags: Add tests and examples replacement for gogo.moretags #382

bwplotka opened this issue Jan 16, 2021 · 12 comments
Labels

Comments

@bwplotka
Copy link
Collaborator

We are removing gogo support from v2. The reason is that gogo is no longer maintained as far as things are right now (Not a surprise: it's a huge backlog of work that overlaps with https://github.com/protocolbuffers/protobuf-go)

This means that we can't have tags interceptor test cases anymore as the things are now. We can still release tags as they were in middleware v1, but there are currently no tests or good example that works. (Let's discuss that in [the PR](#321 @johanbrandhorst @yashrsharma44)

The only question is what's next. https://github.com/protocolbuffers/protobuf-go is not adding such an option, so probably we can use any of community driven efforts like or ensure there is a standard way to enabling tag options somehow. There are a couple of options (See golang/protobuf#52 for full discussion):

Would be nice to decide on before adding tags back to v2. Does this plan make sense?

Help wanted to define what should we use 🤗

@bwplotka bwplotka added the v2 label Jan 16, 2021
@johanbrandhorst
Copy link
Collaborator

Personally I would vote in favour of not making an opinionated decision here and simply not supporting tags until APIv2 does (if ever). I see this project as lightweight and unopinionated, and we should avoid ambiguity where users may be forced to make a choice they don't want to.

On a personal note, I've never used tags, so I'm not sure they're that crucial either?

@bwplotka
Copy link
Collaborator Author

I believe tags are crucial to generate request/operation ID, no? Also we used tags to pass info between middlewares (across network even!). 🤔 We definitely did not use this feature of auto extraction of tags messages though.

I think we are missing good requirements spec (:

From my side, AC:

  • grpc middlewares have a way to extract request/operation ID for logging and tracing purposes from outside (caller) or generate a new one that is missing.
    • Passing info between middlewares / across middlewares across network is kind of critical for request ID passing as well.

@bwplotka
Copy link
Collaborator Author

If we simplify tags to just passing stuff no extraction from types - I think we could have them in v2 or after (:

@johanbrandhorst
Copy link
Collaborator

In my experience you can defer "requestID" generation to your choice of trace provider, for example, opencensus makes that accessible in the context.

@bwplotka
Copy link
Collaborator Author

bwplotka commented Jan 18, 2021 via email

@johanbrandhorst
Copy link
Collaborator

That's the point of tracing, it lives in your context, and is automatically transferred across network boundaries by your trace provider (like ocgrpc).

@bwplotka
Copy link
Collaborator Author

As talked offline, agree. Removing field extraction.

Still torn on tags passing between middleware that only works if Tags interceptor is enabled. It's super confusing. I think I will kill whole interceptor/tag package for v2. We can leave the context passed tags so user can put there tags that can be used for tracing, and logging 🤔 But it will always work - no special tags interceptor needed. It will be clearer when I will propose something in a PR.

@bwplotka
Copy link
Collaborator Author

I can see there are literally two context states:

niceMD and tags

niceMd are metadata trailers (type MD map[string][]string) passed between gRPC peers, tags are internal (map[string]string)

I think it's a good moment to rethink this, unified this and put them in single/separate focused packages that will allow to understand them better.... vs just utils package =D

@bwplotka
Copy link
Collaborator Author

I feel all of this should be part of tracing... But then it makes logging weird as it would use tracing parts 🤔

@johanbrandhorst
Copy link
Collaborator

A logger can extract span and trace IDs from the context so that tags attached to a trace can be mapped to a log message by including the trace ID in any log messages.

@bwplotka
Copy link
Collaborator Author

It can yes... but not sure what we want to have on our middleware then. Kind of sucks to reimplement context logger

bwplotka added a commit that referenced this issue Jan 30, 2021
bwplotka added a commit that referenced this issue Jan 30, 2021
bwplotka added a commit that referenced this issue Aug 6, 2021
…bility.

Fixes #382


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

# Conflicts:
#	go.sum
#	interceptors/client_test.go
#	interceptors/logging/interceptors.go
#	interceptors/logging/payload.go
#	interceptors/logging/payload_test.go
#	interceptors/tags/context.go
#	interceptors/tags/doc.go
#	interceptors/tags/examples_test.go
#	interceptors/tags/fieldextractor.go
#	interceptors/tags/fieldextractor_test.go
#	interceptors/tags/interceptors.go
#	interceptors/tags/interceptors_test.go
#	interceptors/tags/options.go
bwplotka added a commit that referenced this issue Aug 6, 2021
bwplotka added a commit that referenced this issue Aug 6, 2021
bwplotka added a commit that referenced this issue Aug 7, 2021
…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 added a commit that referenced this issue Aug 7, 2021
…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 added a commit that referenced this issue Aug 17, 2021
…bility. (#394)

* Removed tags; Simplified interceptor code; Added logging fields editability.

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]>

* Addressed comments.

Signed-off-by: Bartlomiej Plotka <[email protected]>
@bwplotka
Copy link
Collaborator Author

Tags are removed. gogo.moretags are not supported. Will mention in "migration guide" which is being produced here #543

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants