added system properties to enable logging.#797
Conversation
| type = LoggerOutputType.valueOf(loggerOutputType.toUpperCase()); | ||
| } catch (Exception e) { | ||
| System.out.println(e); | ||
| System.err.println(e); |
There was a problem hiding this comment.
This should be output to stderr and though I could make a case for it being removed.
| } | ||
|
|
||
| private void log(LoggingLevel requestLevel, String message, Object... args) { | ||
| if (!initialized && !systemPropertyInitialize()) { |
There was a problem hiding this comment.
It may not appear so, but this is uses double-checked locking since the method is synchronized
| } | ||
|
|
||
| private boolean initialized = false; | ||
| private volatile boolean initialized = false; |
There was a problem hiding this comment.
I think it's good to turn this to AtomicBoolean and same for the props variable. Then we don't need the sync block.
There was a problem hiding this comment.
AtomicBoolean won't work here because both initialized and propsNotFound need to be guarded by the same lock
There was a problem hiding this comment.
Ok in this case wouldn't it be good to just take an explicit lock and guard this block of code? Why do we need to lock on the whole method. Would that be possible?
There was a problem hiding this comment.
Since the initialize method was synchronized, I kept the new part synchronized as well. I'm not changing the nature of this class, just adding functionality.
| } | ||
| } | ||
|
|
||
| public synchronized boolean systemPropertyInitialize() { |
There was a problem hiding this comment.
This synchronized block is a costly lock operation especially when checking on every log message. It will significantly reduce the performance of SDK when trace logging is turned on`.
Also, I am not sure why a synchronized is needed here. Are there any known scenarios where multiple threads can try to simultaneously initialize this. Atleast not that I know of.
There was a problem hiding this comment.
I think it's fine to keep initialization thread safe. It's only for diagnostics so it doesn't really affect the performance of the SDK.
There was a problem hiding this comment.
Do you envision any scenario for thread safety? Do you see any cases where there can be two threads do initialization at same time. In my mind there are none. Please educate me with one, than I am fine to keep this sync. Otherwise its just wise to remove it imho.
There was a problem hiding this comment.
We can try to determine if thread safety is necessary in a different project. My intention was not to change how this class works, just to add a feature.
| public synchronized boolean systemPropertyInitialize() { | ||
| // if the logger is already initialized, don't init again | ||
| // if the system properties have already been examined, don't check again | ||
| if (propsNotFound || initialized) { |
There was a problem hiding this comment.
By turning these variables to Atomic we can gurantee that initialization happens only once.
There was a problem hiding this comment.
See my comment above on it.
| } | ||
|
|
||
| private void log(LoggingLevel requestLevel, String message, Object... args) { | ||
| if (!initialized && !systemPropertyInitialize()) { |
There was a problem hiding this comment.
I would not recommend calling this method on every log call. Instead what you can do is that move this logic of initialization to, TelemetryConfigurationFactory class. It has the a method setMinimumConfiguration() if there is no config file. Also you can prioritize this initialization over config file if needed, but regardless the method should be called only once instead of calling it everytime on each log message.
There was a problem hiding this comment.
It's only called if not initialized.
dhaval24
left a comment
There was a problem hiding this comment.
I would suggest some reconsideration based on my comments below.
dhaval24
left a comment
There was a problem hiding this comment.
Please consider some comments I responded with.
|
@dhaval24 Let me know if you need more clarification |
dhaval24
left a comment
There was a problem hiding this comment.
@littleaj I think for now we can get this functionality as it provides us good diagnostic info about configuration itself. For future, before merging please create an issue on GH for Threadsaftey in logging so we can get back to it.
Fix #653
This allows a system property to enable logging. This will be especially useful for read log messages which are printed before the config file is read (or if it's non-existant).
This also deprecates the logAlways statement. We've discussed this method before and its pitfalls. I vote to remove it. If you disagree, let's discuss and I can revert it if needed.