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

Allow callers to pass go context through to hooks #559

Merged
merged 1 commit into from Jul 25, 2023
Merged

Conversation

danielbprice
Copy link
Contributor

@danielbprice danielbprice commented Jun 22, 2023

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.

event.go Outdated Show resolved Hide resolved
@danielbprice danielbprice force-pushed the master branch 2 times, most recently from da8d267 to 9fc6f5e Compare June 22, 2023 17:51
@danielbprice
Copy link
Contributor Author

danielbprice commented Jun 22, 2023

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.

@danielbprice danielbprice force-pushed the master branch 2 times, most recently from 9727046 to 3645fde Compare June 22, 2023 18:25
event.go Outdated Show resolved Hide resolved
@rs
Copy link
Owner

rs commented Jun 23, 2023

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?

@danielbprice
Copy link
Contributor Author

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?

Sure thing, should be an easy change. I want you to be satisfied with the interface choices.

@danielbprice
Copy link
Contributor Author

danielbprice commented Jun 26, 2023

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?

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:

  1. (keep what we have in this PR): logger.InfoCtx(ctx).Str("mystr", "abcd").Msg("hello")
  2. (pass at terminal end): logger.Info().Str("mystr", "abcd").MsgCtx(ctx, "hello")
  3. (pass via new function): 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.

@danielbprice
Copy link
Contributor Author

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.

@rs
Copy link
Owner

rs commented Jul 9, 2023

  1. probably makes the most sense. I would probably use WithContext(ctx) and Context() as method names.

@danielbprice
Copy link
Contributor Author

danielbprice commented Jul 13, 2023

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:

  • This implements "option 3" from the discussion above.
  • After a bunch of experimentation, I propose settling on the use of 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.
  • I have pulled this version into my project at work, deployed it to our development environment, and confirmed that I can implement Datadog unified logging/tracing using it.
  • I also tried to improve the documentation for getting and storing Loggers 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.
  • I did not bring in the revised 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.

@danielbprice danielbprice marked this pull request as ready for review July 13, 2023 18:43
@danielbprice danielbprice requested a review from rs July 13, 2023 20:40
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.
@danielbprice
Copy link
Contributor Author

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 rs merged commit 873cbf1 into rs:master Jul 25, 2023
4 checks passed
@danielbprice
Copy link
Contributor Author

@rs Thank you so much! I really appreciate the opportunity to contribute.

@inputvalidation
Copy link

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.

@SophisticaSean
Copy link

Super stoked to use this. Thank you @danielbprice for your contribution here.

@danielbprice
Copy link
Contributor Author

@rs Any chance you would be willing to tag v1.30.0 so that people can start to pull this into projects more easily?

@rs
Copy link
Owner

rs commented Jul 29, 2023

done

@danielbprice
Copy link
Contributor Author

Many thanks!

madkins23 pushed a commit to madkins23/zerolog that referenced this pull request Mar 2, 2024
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.
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

4 participants