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

Add fluent combinators for adding context to structured loggers #425

Merged

Conversation

nigredo-tori
Copy link
Contributor

@nigredo-tori nigredo-tori commented Apr 10, 2021

Closes #424

  • addContext(Map[String, String]) members are trivial wrappers over existing withContext functions. I couldn't call this withContext because for some reason that disabled static forwarders for StructuredLogger.withContext etc., which broke binary compatibility. 🤷 Thankfully, I like the new name better than the old one, because it makes it clear that the context is appended rather than replaced.
  • addContext((String, Shown)*) is sugar over it using the Show.Shows magnet. Consider:
    logger.addContext("foo" -> 123)
    // versus
    logger.addContext(Map("foo" -> 123.show))

We can add similar sugar for error, info etc. - but I'm not sure those would be useful enough to justify the required explosion of the number of methods.

@lorandszakacs
Copy link
Member

lorandszakacs commented Apr 16, 2021

I do like the idea, and it has my OK. But before that:

  1. the feature should branch off of series/1.x branch, and we forward port it to main series 2.x
  2. the method should exist on StructuredLogger[F[_]], and then override it in SelfAwareStructuredLogger[F[_]] as well. The problem is analogous to the withModifiedString method, fixed in Add missing withModifiedString overrides in Logger[F] subclasses #430

@lorandszakacs lorandszakacs added the enhancement New feature or request label Apr 16, 2021
@nigredo-tori nigredo-tori changed the base branch from main to series/1.x April 16, 2021 10:00
@nigredo-tori
Copy link
Contributor Author

  1. the feature should branch off of series/1.x branch, and we forward port it to main series 2.x

I've rebased this PR to 1.x, with no conflicts.

  1. the method should exist on StructuredLogger[F[_]], and then override it in SelfAwareStructuredLogger[F[_]] as well. The problem is analogous to the withModifiedString method, fixed in Add missing withModifiedString overrides in Logger[F] subclasses #430

Could you please elaborate? As far as I see, this is already OK - both methods are introduced in StructuredLogger[F[_]] (returning StructuredLogger[F]), and both are explicitly overriden in SelfAwareStructuredLogger[F[_]] (returning SelfAwareStructuredLogger[F]).

@lorandszakacs
Copy link
Member

Could you please elaborate? As far as I see, this is already OK - both methods are introduced in StructuredLogger[F[_]] (returning StructuredLogger[F]), and both are explicitly overriden in SelfAwareStructuredLogger[F[_]] (returning SelfAwareStructuredLogger[F]).

You're right, sorry. Must have overlooked 😅 all good 👍

@lorandszakacs lorandszakacs merged commit 3c30325 into typelevel:series/1.x Apr 23, 2021
@nigredo-tori nigredo-tori deleted the 424-with-context-sugar branch April 23, 2021 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streamline adding context to structured loggers
3 participants