Allow to configure logback logger with env variables #8998

skabashnyuk · 2018-03-03T07:39:26Z

What does this PR do?

Allow to configure logback logger with env variables
Example.

CHE_LOGGER_CONFIG=org.eclipse.che=DEBUG,org.eclipse.che.api.installer.server.impl.LocalInstallerRegistry=OFF

To be able to configure ws-master - a user will have to add this variable to che.env

In case of ws-agent - a user will have to add this variable to env configuration of the desired machine.

What issues does this PR fix or reference?

Fixes #8997
Depend on eclipse-che/che-lib#79

Release Notes

CHE_LOGGER_CONFIG environment variable is used to configure logback logger

Docs PR

eclipse-che/che-docs#369

sleshchenko · 2018-03-03T07:54:32Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+
+    String config = System.getenv("CHE_LOGGER_CONFIG");
+    if (config != null && !config.isEmpty()) {
+      Arrays.stream(config.split(",")).map(String::trim).forEach(this::setLoggerLevel);


As far as I understand RuntimeException will be thrown in a case when CHE_LOGGER_CONFIG value has the wrong format. Am I right? If yes, maybe it would be better to catch it and log pretty message like 'CHE_LOGGER_CONFIG env variable has the wrong format. It should be ...'

No, AFAIK it would be just silently ignored. In fact, the same way if you put not existed classes or packages in logback.xml

sleshchenko · 2018-03-03T07:57:00Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+import ch.qos.logback.core.spi.LifeCycle;
+import java.util.Arrays;
+
+public class EnvironmentVariablesLogLevelPropagator extends ContextAwareBase


Please add java doc

sleshchenko · 2018-03-03T07:57:52Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+
+  private boolean isStarted;
+
+  private void setLoggerLevel(String loggerConfig) {


It would a bit better if a private method is placed under that one who invokes it

sleshchenko · 2018-03-03T07:58:17Z

assembly/assembly-ide-war/pom.xml

+            <artifactId>che-core-commons-logback</artifactId>
+            <exclusions>
+                <exclusion>
+                    <artifactId>che-core-api-core</artifactId>


Why is it excluded?

to make sure ide war doesn't contain che-core-api-core and all it unnecessary dependencies.

Looks like a bit strange. It means that you're using classes from this dependency which don't use api-core classes.
But somebody can easily use it, and then Runtime exception will occur.
It's just thought. I'm OK to leave it as it is.

Why we need logback in IDE war at all?

@sleshchenko about exclusion.
We already have che-core-commons-logback module. To avoid exclusion we need another module with only one single class. When I selecting this approach I thought it would be less confusing then to have two modules.

Why we need logback in IDE war at all?

@garagatyi
Some filters are using slf4j-> logback logging.

Well, it is more convenient right now, but we will have to remember that this exclusion is needed. And other projects that would extend Che also would need to know about this exclusion.

That would not be a problem soon. Since that is not a case for Theia.

I'm ok with this approach, just thinking out loud

garagatyi · 2018-03-03T09:14:30Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+public class EnvironmentVariablesLogLevelPropagator extends ContextAwareBase
+    implements LoggerContextListener, LifeCycle {
+
+  private boolean isStarted;


Can you elaborate on how it is used and why it should not be thread safe?

It is implemented in similar to ch.qos.logback.classic.jul.LevelChangePropagator way
I guess life cycle of this component is not multi-threaded.

garagatyi · 2018-03-03T09:15:48Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+  }
+
+  private void setLoggerLevel(String loggerConfig) {
+    int i = loggerConfig.indexOf('=');


I believe that call of split() would be clearer.

sleshchenko · 2018-03-03T09:11:09Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+    }
+    String loggerName = loggerConfig.substring(0, i);
+    String levelStr = loggerConfig.substring(i + 1);
+    if (loggerName == null) {


substring method never returns null, these null checks look like redundant. After removing them you'll be able to inline String loggerName = loggerConfig.substring(0, i).trim();

sleshchenko · 2018-03-03T09:16:59Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+    loggerName = loggerName.trim();
+    levelStr = levelStr.trim();
+
+    addInfo("Trying to set level " + levelStr + " to logger " + loggerName);


Maybe it makes sense to check levelStr and loggerName variable on empty here? Or at least wrap them into ' to make it clear that they are empty.

sleshchenko · 2018-03-03T09:30:44Z

assembly/assembly-ide-war/pom.xml

+            <artifactId>che-core-commons-logback</artifactId>
+            <exclusions>
+                <exclusion>
+                    <artifactId>che-core-api-core</artifactId>


Looks like a bit strange. It means that you're using classes from this dependency which don't use api-core classes.
But somebody can easily use it, and then Runtime exception will occur.
It's just thought. I'm OK to leave it as it is.

garagatyi · 2018-03-03T09:46:04Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+    }
+  }
+
+  public void stop() {


stop and isStarted methods are not used. Can we just delete them?

They come from the interface. I'm going to restore Override annotations.

Oh, then it would be clearer

…bles

sleshchenko · 2018-03-03T09:56:12Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+  }
+
+  private void setLoggerLevel(String loggerConfig) {
+    String[] parts = loggerConfig.split("=");


I think it would be better to specify limit here

sleshchenko · 2018-03-03T09:57:18Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+    }
+    String loggerName = parts[0];
+    String levelStr = parts[1];
+    if (loggerName == null) {


When is it possible that parts array contains null values?

…v variables

…with env variables

…logger with env variables

sleshchenko

Thanks for contributing such feature.
Previously setting the log level to DEBUG just enabled chaos mode for logs. 😃
When it is possible to set the log level for a particular component, debug logs become useful.

sleshchenko · 2018-03-03T10:19:51Z

...ck/src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java

+    if (parts.length < 2) {
+      return;
+    }
+    String loggerName = parts[0];


it makes sense to trim values here

skabashnyuk · 2018-03-03T10:23:49Z

ci-test

gazarenkov

Useful, make sure we add this info to documentation.

codenvy-ci · 2018-03-03T15:34:34Z

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:8998
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Allow to configure logback logger with env variables (eclipse-che#8998)

Allow to configure logback logger with env variables

7859a5a

skabashnyuk requested a review from riuvshin as a code owner March 3, 2018 07:39

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Mar 3, 2018

skabashnyuk requested review from gazarenkov, garagatyi, sleshchenko, vparfonov and l0rd March 3, 2018 07:40

skabashnyuk mentioned this pull request Mar 3, 2018

Allow to configure logback logger with env variables eclipse-che/che-lib#79

Merged

sleshchenko reviewed Mar 3, 2018

View reviewed changes

fixup! Allow to configure logback logger with env variables

9699e6f

garagatyi reviewed Mar 3, 2018

View reviewed changes

sleshchenko reviewed Mar 3, 2018

View reviewed changes

fixup! fixup! Allow to configure logback logger with env variables

73d1cdf

garagatyi reviewed Mar 3, 2018

View reviewed changes

fixup! fixup! fixup! Allow to configure logback logger with env varia…

58f03be

…bles

skabashnyuk changed the title ~~[WP] Allow to configure logback logger with env variables~~ Allow to configure logback logger with env variables Mar 3, 2018

sleshchenko reviewed Mar 3, 2018

View reviewed changes

skabashnyuk added 3 commits March 3, 2018 12:04

fixup! fixup! fixup! fixup! Allow to configure logback logger with en…

04a6bef

…v variables

fixup! fixup! fixup! fixup! fixup! Allow to configure logback logger …

92a571b

…with env variables

fixup! fixup! fixup! fixup! fixup! fixup! Allow to configure logback …

10e7091

…logger with env variables

garagatyi approved these changes Mar 3, 2018

View reviewed changes

sleshchenko approved these changes Mar 3, 2018

View reviewed changes

gazarenkov approved these changes Mar 3, 2018

View reviewed changes

gazarenkov approved these changes Mar 3, 2018 •

edited

Loading

View reviewed changes

vparfonov approved these changes Mar 3, 2018

View reviewed changes

skabashnyuk mentioned this pull request Mar 6, 2018

Added information how to configure logback with environment variables eclipse-che/che-docs#369

Merged

skabashnyuk merged commit e8750d6 into master Mar 7, 2018

skabashnyuk deleted the loggbackconfig branch March 7, 2018 11:32

hkolvenbach pushed a commit to hkolvenbach/che that referenced this pull request Mar 12, 2018

Allow to configure logback logger with env variables (eclipse-che#8998)

a20de66

Allow to configure logback logger with env variables (eclipse-che#8998)

garagatyi mentioned this pull request Mar 14, 2018

Port possibility to tune logger level for different components from upstream Che redhat-developer/rh-che#594

Closed

5 tasks

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Mar 16, 2018

benoitf added this to the 6.3.0 milestone Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to configure logback logger with env variables #8998

Allow to configure logback logger with env variables #8998

skabashnyuk commented Mar 3, 2018 •

edited

Loading

sleshchenko Mar 3, 2018

skabashnyuk Mar 3, 2018

sleshchenko Mar 3, 2018

sleshchenko Mar 3, 2018

sleshchenko Mar 3, 2018

skabashnyuk Mar 3, 2018

sleshchenko Mar 3, 2018

garagatyi Mar 3, 2018

skabashnyuk Mar 3, 2018

skabashnyuk Mar 3, 2018 •

edited

Loading

garagatyi Mar 3, 2018

skabashnyuk Mar 3, 2018 •

edited

Loading

garagatyi Mar 3, 2018

garagatyi Mar 3, 2018

skabashnyuk Mar 3, 2018 •

edited

Loading

garagatyi Mar 3, 2018

sleshchenko Mar 3, 2018

sleshchenko Mar 3, 2018

sleshchenko Mar 3, 2018

garagatyi Mar 3, 2018

skabashnyuk Mar 3, 2018

garagatyi Mar 3, 2018

sleshchenko Mar 3, 2018

sleshchenko Mar 3, 2018

sleshchenko left a comment

sleshchenko Mar 3, 2018

skabashnyuk commented Mar 3, 2018

gazarenkov left a comment

codenvy-ci commented Mar 3, 2018


		private boolean isStarted;

		private void setLoggerLevel(String loggerConfig) {

Allow to configure logback logger with env variables #8998

Allow to configure logback logger with env variables #8998

Conversation

skabashnyuk commented Mar 3, 2018 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skabashnyuk commented Mar 3, 2018

gazarenkov left a comment

Choose a reason for hiding this comment

codenvy-ci commented Mar 3, 2018

skabashnyuk commented Mar 3, 2018 •

edited

Loading

skabashnyuk Mar 3, 2018 •

edited

Loading

skabashnyuk Mar 3, 2018 •

edited

Loading

skabashnyuk Mar 3, 2018 •

edited

Loading