Allow logging to be disabled for individual classes #3
Conversation
| @@ -71,12 +71,27 @@ object DynamicOdinConsoleLogger { | |||
|
|
|||
| case class RuntimeConfig( | |||
| minLevel: Level, | |||
| levelMapping: Map[String, Level] = Map.empty | |||
| levelMapping: Map[String, LevelConfig] = Map.empty | |||
There was a problem hiding this comment.
Technically this should have failed the bincompat checks 🤔 as the type has changed, I'll try and work out why that isn't working.
In the meantime, I'm trying to think of the best way to make a bincompat change here and I think you might need to create a new case class that uses the new level config. Then have a smart constructor that takes this old style config and translates it to the new.
There was a problem hiding this comment.
Ah, yes that's a good point. I'll see what I can do, but the bincompat doesn't fail locally for me either, which doesn't make me too confident that changes I make are actually ok.
Could it be that, because it's a type parameter it is technically bincompat because of erasure? That wouldn't seem right to be but....
There was a problem hiding this comment.
Could it be that, because it's a type parameter it is technically bincompat because of erasure?
💯 that's how the JVM works. It won't save your from runtime errors, but it won't be a linking error.
There was a problem hiding this comment.
Just looking at the bin compat guide. Would this maintain bin compat?
case class RuntimeConfig private (
minLevel: Level,
levelMapping: Map[String, Level] = Map.empty,
disabledClasses: List[String] = Nil
) {
def withDisabledClasses(disabled: List[String]) = ???
}
object RuntimeConfig {
def apply(minLevel: Level, levelMapping: Map[String, Level] = Map.empty) = RuntimeConfig(minLevel, levelMapping)
}Maybe the copy method needs fixing too, and not sure how if default args play a role or not
| sealed trait LevelConfig { | ||
| def toLevel: Option[Level] | ||
| } | ||
|
|
||
| object LevelConfig { | ||
| case object Trace extends LevelConfig { val toLevel = Some(Level.Trace) } | ||
| case object Debug extends LevelConfig { val toLevel = Some(Level.Debug) } | ||
| case object Info extends LevelConfig { val toLevel = Some(Level.Info) } | ||
| case object Warn extends LevelConfig { val toLevel = Some(Level.Warn) } | ||
| case object Error extends LevelConfig { val toLevel = Some(Level.Error) } | ||
| case object Off extends LevelConfig { val toLevel = None } | ||
|
|
||
| implicit val eq: Eq[LevelConfig] = cats.derived.semiauto.eq | ||
| } |
There was a problem hiding this comment.
Having an ADT here is nice, but it makes it hard to evolve the config model in a bincompat fashion, I think it might be better to use a value class here with a smart constructor that checks the string value.
| )(make: RuntimeConfig => Logger[F])(implicit eq: Eq[RuntimeConfig]) | ||
| extends DefaultLogger[F](level) | ||
| extends DefaultLogger[F](Level.Trace) |
There was a problem hiding this comment.
This feels a bit dangerous. With this being set, we will inspect the ref on every log message, but it does at least mean that the level mapping can override the default level setting.
I.e. every trace/debug message will hit the ref via the submit implementation below, rather than being filtered beforehand by this level here.
I'm wondering whether we should re-introduce minLevel which is the "absolute" minimum and cannot be lowered at runtime to prevent trace messages from hitting the ref.
| test("lowers default-level config") { | ||
| PropF.forAllF { (messageBeforeChange: String, messageAfterChange: String) => | ||
| val messages = runTest() { logger => | ||
| logger.info(messageBeforeChange) >> | ||
| IO.sleep(10.millis) >> | ||
| logger.update(RuntimeConfig(defaultLevel = Level.Debug)) >> | ||
| logger.debug(messageAfterChange) | ||
| } | ||
| messages.map(_.map(_.message.value).toList).assertEquals(List(message1)) | ||
| messages | ||
| .map(_.map(_.message.value).toList) | ||
| .assertEquals(List(messageBeforeChange, messageAfterChange)) | ||
| } |
There was a problem hiding this comment.
This would have failed in the old implementation as the logging level of DynamicOdinConsoleLoggerImpl would have been set to what ever the original value in RuntimeConfig was. This meant that you could raise the logging level, but never lower it!
The knock on effect of this change is covered in this comment
This adds a new
OFFconfiguration which can be specified for a particular enclosure (ie. such as a class)in theRuntimeConfig.This allows the logging to be completely disabled for that enclosure; previously it was only possible to raise the level up to
ERROR.