include debug as env #890
include debug as env #890
Conversation
|
Reference that explains the naming convention: For settings that cannot make use of config, we should continue this convention. I'm applying it to envvars as well. |
| // we haven't initialized the Agent yet, so we cannot check it in the usual low-cost way by | ||
| // calling Agent.isDebugEnabled(). So we duplicate the functionality here for use in a few cases. | ||
| private static final boolean isNewRelicDebug() { | ||
| final String newrelicDebug = "newrelic.debug"; | ||
| return System.getProperty(newrelicDebug) != null && Boolean.getBoolean(newrelicDebug); | ||
| return Boolean.getBoolean(newrelicDebug) || Boolean.parseBoolean(System.getenv("NEWRELIC_DEBUG")); |
There was a problem hiding this comment.
Boolean.getBoolean already provides a null guard and defaults to false. removed unnecessary null guard.
clean code. no null guard needed clean configfilehelper and settings.gradle fix comment
tbradellis
force-pushed
the
debug-with-sys-or-env
branch
from
a12c51e
to
384e2b2
Compare
| @@ -76,6 +76,11 @@ public static boolean isDebugEnabled() { | |||
| return DEBUG; | |||
| } | |||
|
|
|||
| private static boolean setDebug(){ | |||
There was a problem hiding this comment.
This could be a static initialization block. Or a public static method, that can be used in the other parts of the code that are making the same verification.
There was a problem hiding this comment.
@meiao Are you thinking to include the other static members in the block or just for DEBUG?
Or just:
private static final boolean DEBUG;
static{
DEBUG = setDebug();
//OR use below and drop the method?
//DEBUG =Boolean.getBoolean("newrelic.debug") || Boolean.parseBoolean(System.getenv("NEWRELIC_DEBUG"));
}
isDebugEnabled() is public so other places can already check. Do we want other places to be able to set? It was actually a final variable, so should only set once...unless you think change that?
There was a problem hiding this comment.
I just added setDebug as a private member because I thought it might look cleaner to move the boolean checks out of that section since there are multiple checks.
There was a problem hiding this comment.
My first idea was to drop the method.
But as I read thru the PR, there are 3 other places that make the same logic:
Boolean.getBoolean(newrelicDebug) || Boolean.parseBoolean(System.getenv("NEWRELIC_DEBUG"));
So instead of copying that all the time, it could be a method that all call.
There was a problem hiding this comment.
I see. One of the problems with one of the places is that Agent class hasn't been initialized when it is called. It was unclear to me why the original author didn't decide to make static call from AgentJarHelper where they had to duplicate it.
Do you think leave AgentJarHelper as is and just the Agent class in the other places where we can? Or something else?
There was a problem hiding this comment.
OH, using it in AgentJarHelper would initialize it wouldn't it.
| @@ -50,7 +50,7 @@ include 'functional_test' | |||
| include 'functional_test:weave_test' | |||
| include 'functional_test:weave_test_impl' | |||
|
|
|||
| // JDK 9+ module system tests | |||
| JDK 9+ module system tests | |||
There was a problem hiding this comment.
Why uncomment this line?
Ah, guess you were uncommenting the lines below this and accidentally uncommented this one.
The config file will not work for newrelic.debug. Some of the checks happen prior to ServiceManager and ConfigService init. The property must be accessible prior to that setup.
Of note, the AgentConfigImpl still maintains variables for this debug setting. However, nothing is ever used from the AgentConfigImpl. From the looks of it, this was done because AgentConfigImpl implements AgentConfig which requires the implementation detail for isDebugEnabled. I surmise that at some point a decision was made to separate debug from AgentConfig because a need arose to log either to standard out or otherwise before the ConfigService was initialized.
The actual newrelic.debug implementation happens on Agent. It is also duplicated on AgentJarHelper. There are assumed historical reasons for this, and for the sake of time we decided to not refactor. It makes more sense to keep the current implementation and just extend it to also check for an environment variable in all the places that we currently check for the system property.