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

logging/zap/ctxzap: add shorthand functions #408

Merged

Conversation

CAFxX
Copy link
Contributor

@CAFxX CAFxX commented Apr 9, 2021

Adds common shorthand functions. See #407 for details.

@CAFxX CAFxX force-pushed the cafxx-ctxzap-shorthand branch 2 times, most recently from 62d7509 to 7360fbc Compare April 9, 2021 05:58
Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johanbrandhorst johanbrandhorst merged commit 315ddd9 into grpc-ecosystem:master Apr 9, 2021
@johanbrandhorst
Copy link
Collaborator

Could you have a look at cherry-picking this to the v2 branch? It may need some rejigging.

@CAFxX CAFxX deleted the cafxx-ctxzap-shorthand branch April 14, 2021 13:14
@CAFxX
Copy link
Contributor Author

CAFxX commented Apr 14, 2021

@johanbrandhorst it seems the whole package is missing in v2. Has it been left out on purpose, or it's simply still to be ported over?

@johanbrandhorst
Copy link
Collaborator

The logging infrastructure was refactored significantly in v2, but maybe https://github.com/grpc-ecosystem/go-grpc-middleware/blob/v2/providers/zap/logger.go is the place?

@CAFxX
Copy link
Contributor Author

CAFxX commented Apr 15, 2021

Yeah saw that, but it seems all of those providers are missing the ctx* subpackage (like ctxzap here). Is it deliberate to not have the context based functionality, or just it hasn't been done yet?

(One valid reason for not doing it may be the desire to write a generic-based implementation that works with any provider)

@johanbrandhorst
Copy link
Collaborator

CC @bwplotka

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

Successfully merging this pull request may close these issues.

None yet

2 participants