Closed danielbprice closed 1 year ago
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 Context
method on the event, add an alternative Hook interface that would take a ctx
as the first argument?
Thinking about the API, what do you think about, instead of exposing a new
Context
method on the event, add an alternative Hook interface that would take actx
as the first argument?
Sure thing, should be an easy change. I want you to be satisfied with the interface choices.
Thinking about the API, what do you think about, instead of exposing a new
Context
method on the event, add an alternative Hook interface that would take actx
as the first argument?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 Hook
into a new HookCtx
interface-- we can't add e.g. RunCtx
to the Hook interface. I've put the changes up on a branch for you to see, but it seems awkward to duplicate so much code and mechanism to get this. https://github.com/danielbprice/zerolog/commit/42e0eb265132d8575e8927babdb2856011664544. What do you think?
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: MsgCtx(ctx, "hello")
and MsgfCtx(ctx, "hello %s", "there")
. Or into a method on the event (ex: logger.Warn().Str("foo", "bar").Ctx(ctx).Msg("hello")
? You've had much more time to think about what is right for zerolog, so I am open to refining this some more. In my mind, there are competing forces: .InfoCtx()
feels somewhat more Go-Idiomatic and aligns with slog
. But .Ctx()
feels more idiomatic to the method-chaining style and more "native" to zerolog (IMO).
So, three options I see:
logger.InfoCtx(ctx).Str("mystr", "abcd").Msg("hello")
logger.Info().Str("mystr", "abcd").MsgCtx(ctx, "hello")
logger.Info().Str("mystr", "abcd").Ctx(ctx).Msg("hello")
Any thoughts? It seems to me that either #1 or #3 is most likely to be the right answer.
Any thoughts? It seems to me that either #1 or #3 is most likely to be the right answer.
@rs Just wondered if you have any new thoughts here? I've just wrapped up another project and might have a day to devote to this in the coming week.
WithContext(ctx)
and Context()
as method names.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:
Ctx
when referring to the go context. This is to try to avoid confusion with zerolog.Context. Also, I foresee typing log.Info().Ctx(ctx).Msg("whatever")
a lot, so the brevity is nice. Since ctx
is almost universally the name used for variables of type context.Context
, I think this will feel natural to most programmers. I know you suggested using WithContext(ctx)
. But it seemed confusing to have the already-existing Logger.WithContext()
(which is about storing the logger to the go context) and a newly added Event.WithContext() which does something substantially different.Logger
s to the Go context, so that new users will have a clear idea of what the two different features are, and when to use them.HookCtx
interface that I prototyped in https://github.com/danielbprice/zerolog/commit/42e0eb265132d8575e8927babdb2856011664544. It just seemed really awkward to me, and a lot of code for a relatively small benefit. If there was ever a v2
of the interface, then I would suggest revising Hook
to take a context.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 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.