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 a way to create log without side effects #116

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

FunFunFine
Copy link

Currently LogOf creates desired Log instances with a side-effect. But there is a simple way to avoid that and get plain Log[F], not F[Log[F]]. This is possible because of the fact that both SLF4J and Logback cache their loggers (which LogOf relies on).
This means that it is totally fine to call loggerFactory.getLogger(source) on each log message, because the overhead is simply a hash table call and a couple of if-s.

The names and the details in this PR are subject to change, let's discuss it all.

@@ -76,7 +78,92 @@ object Log {

def summon[F[_]](implicit F: Log[F]): Log[F] = F

def apply[F[_]: Sync](logger: Logger): Log[F] = new Log[F] {
def cached[F[_] : Sync](source: String, factory: ILoggerFactory): Log[F] = new Log[F] {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is still an effectful unpure code, hence - NO from my side. however you can already use logOf(…).toTry.get in your code base, which will clearly express to readers unsafeness, and this is easy to use

Copy link
Author

Choose a reason for hiding this comment

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

Why is that an effectual code?

Copy link
Author

Choose a reason for hiding this comment

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

If you worry about getting the factory in safe manner then check out test file in this PR. It shows that we still should get ILoggerFactory by delaying slf4j stuff.

Copy link
Contributor

@t3hnar t3hnar Nov 16, 2021

Choose a reason for hiding this comment

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

because any manipulations with factory are not safe, and in runtime it really references mutable shared state and changes it

Copy link
Author

@FunFunFine FunFunFine Nov 16, 2021

Choose a reason for hiding this comment

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

Well, I see what you mean. It easily can be solved by putting function here (it should be done anyway because of Logback) like so

def cached[F[_] : Sync](source: String, factory: String => F[Logger]): Log[F] = ???

or by putting factory inside of Ref. I guess it would be pure code then, right?

Copy link
Author

Choose a reason for hiding this comment

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

Also I see that every manipulation with factory is delayed here, isn't it enough to be pure?

Copy link
Contributor

Choose a reason for hiding this comment

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

right!

however such changes would enforce your Log implementation to call factory.getLogger as part of log.info/debug/etc - adds some runtime overhead

Copy link
Author

@FunFunFine FunFunFine Nov 16, 2021

Choose a reason for hiding this comment

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

Such runtime overhead is so little we can ignore it. If you dive in (not so deep) to the implementation, you can see that:

private Map<String, Logger> loggerCache;
//...
public final Logger getLogger(final String name) {

        if (name == null) {
            throw new IllegalArgumentException("name argument cannot be null");
        }

        // if we are asking for the root logger, then let us return it without
        // wasting time
        if (Logger.ROOT_LOGGER_NAME.equalsIgnoreCase(name)) {
            return root;
        }

        int i = 0;
        Logger logger = root;

        // check if the desired logger exists, if it does, return it
        // without further ado.
        Logger childLogger = (Logger) loggerCache.get(name);
    //^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this line   
        // if we have the child, then let us return it without wasting time
        if (childLogger != null) {
            return childLogger;
        }

        // if the desired logger does not exist, them create all the loggers
        // in between as well (if they don't already exist)

//....

}

Copy link
Author

Choose a reason for hiding this comment

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

You know when you create instances of IO and flatMap classes for every operation you make this overhead seems totally fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure it might be totally fine, it depends on actual implementation :)

def cached[F[_] : Sync](source: String, factory: String => F[Logger]): Log[F]

signaturewise & typewise - is ok

@FunFunFine
Copy link
Author

One important thing to discuss is how to integrate it in existing code.
If LogOf had two type-parameters, then it would be simpler: just make a LogOf[Id, F] returning method. But it doesn't hence we need to create another one.
Should we even force the migration to this new version, if it is any good?

@t3hnar
Copy link
Contributor

t3hnar commented Nov 16, 2021

trait MyClass[A[_], B[_], C[_]] {
  def a: A[…]
  def b: B[…]
  def c: C[…]
}

gives the most flexibility, however usually you just do not need that, basically single F[_] is the most common case without over complications. Different might not even give you composability :)

hence I'd recommend to NOT overcomplicate, imho .toTry.get will be simpler :)

@FunFunFine
Copy link
Author

FunFunFine commented Nov 16, 2021

hence I'd recommend to NOT overcomplicate, imho .toTry.get will be simpler :)

I do not want to make my code impure. .toTry.get won't work for me unfortunately.

@FunFunFine
Copy link
Author

```scala
trait MyClass[A[_], B[_], C[_]] {
  def a: A[…]
  def b: B[…]
  def c: C[…]
}

gives the most flexibility, however usually you just do not need that, basically single F[_] is the most common case without over complications.

Regarding multiple type parameters: I was talking about such approach:

trait LogOf[I[_], F[_]] {
  def apply(source: String): I[Log[F]]
}

where I denotes your initialisation effect and F is your main effect where your app runs.
For example, I =:= IO & F =:= ReaderT[IO, MyRequestContext, *], when your app runs in some contextual environment (e.g. context extracted from HTTP request).

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.

3 participants