-
Notifications
You must be signed in to change notification settings - Fork 567
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
Allow callers to pass go context through to hooks #559
Conversation
I added a bunch of testing today, as I didn't want to be less than thorough; however, if they seem like overkill I am happy to yank them. |
Thinking about the API, what do you think about, instead of exposing a new |
Sure thing, should be an easy change. I want you to be satisfied with the interface choices. |
Hi Olivier, So this turned out to be quite a bit more awkward than I had expected, as it required duplicating almost all of the Also, over the weekend, I began to wonder if I spent too little time thinking through the interface here. Would we be better off passing the context directly into the terminal routines-- as in: So, three options I see:
Any thoughts? It seems to me that either #1 or #3 is most likely to be the right answer. |
|
Hi! Sorry for the delay, but work has been busy and I've had to deal with Python structured logging. 🤮 😡 🤬 I posted a significantly revised review:
|
Add Ctx(context.Context) to Event and Context, allowing log.Info().Ctx(ctx).Msg("hello"). Registered hooks can retrieve the context from Event.GetCtx(). Facilitates writing hooks which fetch tracing context from the go context.
Hi @rs, any chance you could take a look at this (significantly) revised version? I have some cycles this week for another round of review and fixing, and hopefully integration. |
@rs Thank you so much! I really appreciate the opportunity to contribute. |
Really appreciate this PR. I was writing my code against it and then found it was not released yet 😇 Not sure of zerolog's release policy but would be great to be able to use this in the field. |
Super stoked to use this. Thank you @danielbprice for your contribution here. |
@rs Any chance you would be willing to tag v1.30.0 so that people can start to pull this into projects more easily? |
done |
Many thanks! |
Add Ctx(context.Context) to Event and Context, allowing log.Info().Ctx(ctx).Msg("hello"). Registered hooks can retrieve the context from Event.GetCtx(). Facilitates writing hooks which fetch tracing context from the go context.
Add Logger.{Trace,Debug,Info,Warn,...}Ctx() and similar functions to allow go context to propagate to Hooks. Add Event.Context() to make the context retrievable by hooks. Facilitates writing hooks which fetch tracing context from the go context.
This PR helps to address #395, #558 and maybe #290. It is modeled off of the similar interfaces in the new
slog
stdlib package in go 1.21.