Initial attempt at a logger that allows deferred conditional logging

[morgen-peschke]

Open questions:

1. Is there a better way to do this?
2. Can this be made to work with LoggerFactory? Yes

Related to Issue #834

[rossabaker]

Would this have been less painful if the levels were a sealed trait, and we didn't need multiple DeerredLogMessage implementations, multiple case statements, etc? Maybe not, because you have to dispatch to the right method on Logger?

How worried should we be about the unbounded size of the Chain? I don't know what to do about it other than premature logging or dropped logging.

[morgen-peschke]

Would this have been less painful if the levels were a sealed trait, and we didn't need multiple DeerredLogMessage implementations, multiple case statements, etc? Maybe not, because you have to dispatch to the right method on Logger?

It would probably be about the same. At some point it might be worth collapsing all of these into a single LogMessage trait that provides a superset of the needed functionality, and just don't bother to use it when not needed (e.g. Logger would ignore the context field).

I don't really think this merits the major version bump that removing the existing classes would require, but it's unquestionably tech debt.

How worried should we be about the unbounded size of the Chain? I don't know what to do about it other than premature logging or dropped logging.

That's a good question that I don't really have a concrete answer for. Deferring anything implies some sort of buffer, and it's kind of up to the user to make sure they're not logging so much stuff that it would cause a problem. It might be worth adding something to the scaladoc calling that out caveat.

[morgen-peschke]

@rossabaker I realized I was being kind of lazy about it, so I converted the old LogMessage class into a sealed trait and folded the new log message classes into the old one.

There's still the ones hanging around for the testing loggers, but those are old so I left them alone.

[morgen-peschke]

Never mind, TIL that converting a case class to a sealed trait isn't a thing in Scala 3, so I reverted those

[rossabaker]

The size of this feels overwhelming for the simplicity of the added functionality, but I don't see an easier and compatible way, and the functionality seems useful. 👍

[morgen-peschke]

The size of this feels overwhelming for the simplicity of the added functionality, but I don't see an easier and compatible way, and the functionality seems useful. 👍

Same 😅
I can see a couple ways to simplify things down, but they all involve breaking bin-compat. I'd be interested in making a pass over this the next time a major version bump is in the works.

[rossabaker]

I'll leave this open a day or two in case anyone else wants to wade in, but looks good to me.

[danicheg]

That's huge!
Log4cats is notoriously known for having a sort of complicated hierarchy of interfaces that confuses some newcomers. So I'm wondering if we could not bring new interfaces DeferredLogger, DeferredLoggerFactory, DeferredSelfAwareStructuredLogger and DeferredStructuredLogger, and devise DeferredSelfAwareStructuredLogger / DeferredStructuredLogger as implementations of SelfAwareStructuredLogger / StructuredLogger with some extra functionality via corresponding 'smart' constructors. WDYT?

[danicheg]

DeferredLoggerFactory depends on DeferredSelfAwareStructuredLogger. It feels a bit non-shapely. Also, it makes me think that perhaps we don't need the DeferredLoggerFactory at all... But maybe I'm overlooking something simple.

[morgen-peschke]

This ended up paralleling a similar dependency in LoggerFactory, as that conceptually depends on SelfAwareStructuredLogger (it's a bit hard to see in the code, but LoggerType is a big clue).

We need DeferredLoggerFactory because, the way LoggerFactory is written, it's near enough impossible to create a LoggerFactory that emits DeferredSelfAwareStructuredLoggers that are aware that they are DeferredSelfAwareStructuredLoggers rather than vanilla SelfAwareStructuredLoggers - which then makes it impossible to manually trigger logging for errors that don't involve an error embedded in F[_]

[morgen-peschke]

That's huge! Log4cats is notoriously known for having a sort of complicated hierarchy of interfaces that confuses some newcomers. So I'm wondering if we could not bring new interfaces DeferredLogger, DeferredLoggerFactory, DeferredSelfAwareStructuredLogger and DeferredStructuredLogger, and devise DeferredSelfAwareStructuredLogger / DeferredStructuredLogger as implementations of SelfAwareStructuredLogger / StructuredLogger with some extra functionality via corresponding 'smart' constructors. WDYT?

I'm sure that, if we're able to do breaking changes, then we'd be able to streamline this considerably.

That being said, I don't think we can get away with a completely folded hierarchy because of the need to expose the log and inspect methods.

[danicheg]

I'm sure that, if we're able to do breaking changes, then we'd be able to streamline this considerably.

I mean, we could have something like this

trait InspectLogging[F[_]] {
  def inspect: F[Chain[DeferredLogMessage]]
  def log: F[Unit]
}

object StructuredLogger {
  def makeDeferred[F[_]]: StructuredLogger[F] with InspectLogging[F] = ???
}

Do we actually need new subtypes like DeferredStructuredLogger? The only reason to have distinct types is to require the usage of those specific Loggers in the downstream code (in argument lists, e.g.). But is this the case?

[morgen-peschke]

I'm sure that, if we're able to do breaking changes, then we'd be able to streamline this considerably.

I mean, we could have something like this

trait InspectLogging[F[_]] {
  def inspect: F[Chain[DeferredLogMessage]]
  def log: F[Unit]
}

object StructuredLogger {
  def makeDeferred[F[_]]: StructuredLogger[F] with InspectLogging[F] = ???
}

Do we actually need new subtypes like DeferredStructuredLogger? The only reason to have distinct types is to require the usage of those specific Loggers in the downstream code (in argument lists, e.g.). But is this the case?

I'm not really sure it'll make much of a difference in terms of the volume of the code, other than potentially making the type signatures longer 🤷🏻

I would be a bit reluctant to put the implementations into the companion objects of the existing loggers, for two reasons:

1. This isn't really core functionality, so having it part of the core classes doesn't really make sense to me.
2. The files for the core classes are already quite large and repetitive, and including the code for the deferred variants would exacerbate that considerably. For example, SelfAwareStructuredLogger's companion object is already about 150 lines of very repetitive code, including DeferredSelfAwareStructuredLogger's implementations would add around 260 lines (most of which look very similar to what's already there).

The thoughts I had about streamlining this via breaking changes would be to try and figure out how to take advantage of the general shape that e.g. DeferredSelfAwareStructuredLogger and ModifiedContextSelfAwareStructuredLogger share to see if we could provide common LoggerTransformer to abstract away all the dispatching we're currently needing to do.

[danicheg]

I'm not worrying about reducing code size or having code DRY at max. I'm rather worried about adding new abstractions. DeferredSelfAwareStructuredLogger has only two new methods and a slightly different semantic compared to the existing API.

[morgen-peschke]

I'm not worrying about reducing code size or having code DRY at max.

I am, I messed up the dispatching a bunch of times, that's why there are so many tests 😅

I'm rather worried about adding new abstractions. DeferredSelfAwareStructuredLogger has only two new methods and a slightly different semantic compared to the existing API.

Fair enough, I'll see what I can do with it. We're under a heat advisory and that makes it hard to get anything done, so I wouldn't expect to see any movement before the weekend.

[danicheg]

JFTR, I don't want to be a party pooper! My comments should be considered mostly as recommendations, and Ross's approval is enough to move on.

[morgen-peschke]

JFTR, I don't want to be a party pooper! My comments should be considered mostly as recommendations, and Ross's approval is enough to move on.

No worries. A bit of surprise good/bad news: we probably do need the subclasses.

Without them the ability to log on demand is lost at the first addContext or withModifiedString, because those are hard-coded to the various logger types and the overridden types are lost when they're implemented as anonymous classes.

The good news is that the process of figuring out raised a missed override of mapK and friends for DeferredLogger and DeferredStructuredLogger, and an opportunity to combine the new logger classes.

As an aside, how easy it was to miss that these were missed is an example of why I'd like to figure out if there's a better way to abstract over the various ways that loggers are transformed. It's just way too easy to get wrong.

[morgen-peschke]

I think I may have found a way to improve the experience of transforming loggers, it's currently in PR in my fork (due to merge target shenanigans), but I'll convert it to a PR here once this is merged.