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

ZIO-2 logging (Part 1) #1038

Merged
merged 3 commits into from
Nov 25, 2022
Merged

ZIO-2 logging (Part 1) #1038

merged 3 commits into from
Nov 25, 2022

Conversation

vagroz
Copy link
Collaborator

@vagroz vagroz commented Nov 15, 2022

This PR copy-pastes ZIO-1 implementation with small improvements.
The docs and ZIO-Native implementation will be brought with another PR.

private implicit val requestIdLoggable: Loggable[String] =
Loggable.stringValue.named("requestId")

override def getCtx: UIO[LoggedValue] = ref.get.map(x => x)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any other way to get implement this method other than that? It feels like any implementation of ContextProvider will be doing something.something.map(x => /*implicit conversion*/ x). Perhaps it's better to make it ContextProvider[A: Loggable]?

Copy link
Collaborator Author

@vagroz vagroz Nov 18, 2022

Choose a reason for hiding this comment

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

ZLogs.layerPlainWithContext is pretty now: we can provide to it any implementation of ContextProvider.
If we make this trait as ContextProvider[A: Loggable], the layer will also require type-parameter which can be huge enough. For example, next PR will bring ContextProvider with data A = Map[ZLogAnnotation[_], Any].

As a user, I don't want write THIS type A, ideally I should not know anything about it. I just write two things

zioProgram.provide(
  ZLogs.layerPlainWithContext,  // dear tofu, give me a logger factory using ContextProvider I point next to
  TofuDefaultContext.layer         // I'd like to log this context, don't carry how this collected
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will be doing something.something.map(x => /implicit conversion/ x)

suggest abstract class ValueContextProvider which do this itself

Copy link
Member

Choose a reason for hiding this comment

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

suggest abstract class ValueContextProvider which do this itself

yep, that'll work

@vagroz vagroz requested review from FunFunFine and removed request for Odomontois, danslapman, oskin1 and catostrophe November 18, 2022 16:03
@FunFunFine FunFunFine merged commit 55ed568 into tofu-tf:master Nov 25, 2022
@FunFunFine FunFunFine mentioned this pull request Nov 25, 2022
@Odomontois Odomontois added the enhancement New feature or request label Nov 30, 2022
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.

3 participants