Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

include debug as env #890

Merged
merged 4 commits into from
Jun 29, 2022
Merged

include debug as env #890

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
384e2b2
include debug as env
tbradellis Jun 22, 2022
79ed317
refactor DebugFlag
tbradellis Jun 27, 2022
febf9c2
clean code
tbradellis Jun 27, 2022
876d043
delete semicolon
tbradellis Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion newrelic-agent/src/main/java/com/newrelic/agent/Agent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public final class Agent {
private static final String NEWRELIC_BOOTSTRAP = "newrelic-bootstrap";
private static final String AGENT_ENABLED_PROPERTY = "newrelic.config.agent_enabled";

private static final boolean DEBUG = Boolean.getBoolean("newrelic.debug");
private static final boolean DEBUG = Agent.setDebug();
private static final String VERSION = Agent.initVersion();

private static long agentPremainTime;
Expand All @@ -76,6 +76,11 @@ public static boolean isDebugEnabled() {
return DEBUG;
}

private static boolean setDebug(){
Copy link
Contributor

@meiao meiao Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tbradellis tbradellis Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

newrelic-java-agent/newrelic-agent/src/main/java/com/newrelic/agent/config/AgentJarHelper.java

Line 227 in 0638d86

// we haven't initialized the Agent yet, so we cannot check it in the usual low-cost way by

Do you think leave AgentJarHelper as is and just the Agent class in the other places where we can? Or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OH, using it in AgentJarHelper would initialize it wouldn't it.

return Boolean.getBoolean("newrelic.debug") || Boolean.parseBoolean(System.getenv("NEWRELIC_DEBUG"));
}


private static volatile boolean canFastPath = true;

/**
Expand Down
2 changes: 1 addition & 1 deletion newrelic-agent/src/main/java/com/newrelic/agent/config/AgentConfigImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ private AgentConfigImpl(Map<String, Object> props) {
putForDataSend = getProperty(PUT_FOR_DATA_SEND_PROPERTY, DEFAULT_PUT_FOR_DATA_SEND_ENABLED);
isApdexTSet = getProperty(APDEX_T) != null;
apdexTInMillis = (long) (getDoubleProperty(APDEX_T, DEFAULT_APDEX_T) * 1000L);
debug = Boolean.getBoolean(DEBUG);
debug = Boolean.getBoolean(DEBUG) || Boolean.parseBoolean(System.getenv("NEWRELIC_DEBUG"));
metricDebug = initMetricDebugConfig();
enabled = getProperty(ENABLED, DEFAULT_ENABLED) && getProperty(AGENT_ENABLED, DEFAULT_ENABLED);
experimentalRuntime = allowExperimentalRuntimeVersions();
Expand Down
4 changes: 2 additions & 2 deletions newrelic-agent/src/main/java/com/newrelic/agent/config/AgentJarHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,12 @@ public static String getAgentJarAttribute(String name) {
}
}

// The "newrelic.debug" flag redirects all Agent logging to the standard output. Unfortunately,
// The "newrelic.debug" flag redirects all Agent logging (prior to log init) to the standard output -after log init, log as usual. Unfortunately,
// 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"));
Copy link
Contributor Author

@tbradellis tbradellis Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boolean.getBoolean already provides a null guard and defaults to false. removed unnecessary null guard.

}

// Use of this method should be limited to serious error cases that would cause the Agent to
Expand Down
4 changes: 3 additions & 1 deletion newrelic-agent/src/main/java/com/newrelic/agent/config/ConfigFileHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public class ConfigFileHelper {
private static final String NEW_RELIC_HOME_DIRECTORY_PROPERTY = "newrelic.home";
private static final String NEW_RELIC_HOME_DIRECTORY_ENVIRONMENT_VARIABLE = "NEWRELIC_HOME";
private static final String NEW_RELIC_DEBUG_PROPERTY = "newrelic.debug";

private static final String NEW_RELIC_DEBUG_ENV = "NEWRELIC_DEBUG";
private static final String[] SEARCH_DIRECTORIES = { ".", "conf", "config", "etc" };

/**
Expand All @@ -36,7 +38,7 @@ public static File findConfigFile() {

File parentDir = getNewRelicDirectory();
if (parentDir != null) {
if (Boolean.getBoolean(NEW_RELIC_DEBUG_PROPERTY)) {
if (Boolean.getBoolean(NEW_RELIC_DEBUG_PROPERTY) || Boolean.parseBoolean(System.getenv(NEW_RELIC_DEBUG_ENV)) ) {
System.err.println(MessageFormat.format("New Relic home directory: {0}", parentDir));
}
}
Expand Down