Log control #90
Log control #90
Conversation
We found LD was to verbose, especially with warn messages. This will let us (your customers) have fine grained controll of what gets logged.
| FeatureFlag featureFlag = config.featureStore.get(featureKey); | ||
| if (featureFlag == null) { | ||
| logger.warn("Unknown feature flag " + featureKey + "; returning default value"); | ||
| logger.debug("Unknown feature flag " + featureKey + "; returning default value"); |
There was a problem hiding this comment.
I agree with this change-- we've seen enough cases where an unknown FF is expected (e.g. test scenarios), so debug does feel appropriate to me.
|
|
||
| private static AtomicReference<ILoggerFactory> factory = new AtomicReference<>(); | ||
|
|
||
| public static void install(ILoggerFactory factory) { |
There was a problem hiding this comment.
@keynan What do you think about making the LoggerFactory part of the config? Then this method doesn't need to be public, and it makes configuring the Logger consistent with the rest of the LDClient setup.
If there's a reason you need this to be configurable after the client has been created, then I'm curious..
(Depending on your logging framework, log levels can be changed at runtime outside of any code changes to this library)
There was a problem hiding this comment.
Oh! I would much rather have this be part of a constructor somewhere. This way was about expediency to start the conversation. I can amend the PR on Monday. How long, do you think, before we'd see it in a release?
There was a problem hiding this comment.
In many cases loggers where instantiated in classes that did not refrence the config so I've left them as is. Where possible it now goes to the LDConfig to get a logger instance.
I couldn't get gradle to build locally so you should double check this all works before merging :/ sorry for the hassle
Couldn't build locally, so please confirm compilation.
|
Looking good! |
|
We're still targeting Java 1.7 for maximum compatibility- that's at least one source of the build error. I'll take a look at fixing it. |
|
@keynan I reworked some things so there's less new code- The LDConfig class now has a static method called getLogger() that we're using to get the Logger. It defaults to the SLF4J LoggerFactory instance (previous behavior) or will use a custom LoggerFactory if the builder has one set. We've pushed 2.2.0-SNAPSHOT, do you mind reviewing these changes and trying out the new version? |
…ith-reasons evaluation reasons can be included in allFlagsState()
4 participants
This commit adds a little indirection around logger construction.
We found LD was to verbose, especially with warn messages.
This will let us (your customers) have fine grained control of what gets logged.
Aside: my personal position is there are exactly 2 log levels: fatal and debug. Everything else is just noise.