Modified logging #365
Modified logging #365
Conversation
marcin-cebo
requested review from
kleewho,
jguz-pubnub,
parfeon and
jakub-grzesiowski
as code owners
| */ | ||
| val httpLoggingInterceptor: HttpLoggingInterceptor? | ||
| @Deprecated( | ||
| message = "This setting is deprecated. Use customLoggers instead", |
There was a problem hiding this comment.
Nitpick: There's an extra space between customLoggers and instead
There was a problem hiding this comment.
Good catch. I will fix that.
| message = "This setting is deprecated. Use customLoggers instead", | ||
| level = DeprecationLevel.WARNING | ||
| ) | ||
| val httpLoggingInterceptor: HttpLoggingInterceptor? // todo deprecate |
There was a problem hiding this comment.
Perhaps // todo deprecate comment is no longer required
There was a problem hiding this comment.
Of course. Good catch. I will fix that.
There was a problem hiding this comment.
Good catch. I will fix that.
| * @see [HttpLoggingInterceptor] | ||
| */ | ||
| @Deprecated( | ||
| message = "This setting is deprecated. Use customLoggers instead", |
There was a problem hiding this comment.
The same nitpick as above, there's an extra space between customLoggers and instead
There was a problem hiding this comment.
Good catch. I will fix that.
| pnConfiguration.retryConfiguration(RetryConfiguration.None.INSTANCE); | ||
| pnConfiguration.logVerbosity(PNLogVerbosity.NONE); | ||
| pnConfiguration.httpLoggingInterceptor(createInterceptor()); | ||
| // pnConfiguration.httpLoggingInterceptor(createInterceptor()); // todo remove when logging is implemented |
There was a problem hiding this comment.
Is this comment up to date?
There was a problem hiding this comment.
No. I will remove it.
| } | ||
|
|
||
| // this method is used from cryptoModuleWithLogConfig using reflection | ||
| fun getCryptoModuleWithLogConfig(logConfig: LogConfig): CryptoModule? { |
There was a problem hiding this comment.
What's the purpose of this method? Is it to inject logConfig property to CryptoModule?
There was a problem hiding this comment.
Why customer creates cryptoModule it create it without logConfig. It is hidden from them. Then we need to add logConfig so that cryptoModule operations can be logged.
| override fun presenceTimeout(presenceTimeout: Int): Builder { | ||
| this.presenceTimeout = if (presenceTimeout < MINIMUM_PRESENCE_TIMEOUT) { | ||
| log.warn("Presence timeout is too low. Defaulting to: $MINIMUM_PRESENCE_TIMEOUT") | ||
| println("Presence timeout is too low. Defaulting to: $MINIMUM_PRESENCE_TIMEOUT") |
There was a problem hiding this comment.
Is it possible to inject logger property in PNConfigurationImpl?
There was a problem hiding this comment.
Unfortunately not. To create logger we need to know logConfig which is created when creating PubNub.
Current order is:
- Create PnConfiguration
- Create instanceId while creating PubNub
- Create logger
| override val authKey: String = "", | ||
| override val authToken: String? = null, | ||
| override val cryptoModule: CryptoModule? = null, | ||
| override val cryptoModule: CryptoModule? = null, // don't use getter directly use getCryptoModuleWithLogConfig to be able to properly configure logging in CryptoModule |
There was a problem hiding this comment.
Is this somehow customer-facing?
There was a problem hiding this comment.
This is internal note. This is hidden in implementation module.
Customers will interact with an interface and they are supposed to set clean cryptoModule. Internally we add to it LogConfig so it is possible to log info about crypto module operations.
|
@pubnub-release-bot release kotlin as v10.6.0 |
|
🚀 Release successfully completed 🚀 |
feat: Added structured log statements throughout the system in transport layer, crypto module, serialization, API calls, etc.
feat: Added deprecation warning for logVerbosity and httpLoggingInterceptor config params
Added deprecation warning for logVerbosity and httpLoggingInterceptor config params. For logging configuration we advise using SLF4J implementation lib like logback or log4j2 and their config capabilities.
feat: Added new config property called customLoggers
Added new config property called customLoggers, which can bu used when customer needs can not be implemented using configuration options of SLF4J implementation libs like logback or log4j2.