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

Allow to configure logback logger with env variables #8998

Merged
merged 7 commits into from
Mar 7, 2018
Merged

Allow to configure logback logger with env variables #8998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7859a5a
Allow to configure logback logger with env variables
skabashnyuk Mar 2, 2018
9699e6f
fixup! Allow to configure logback logger with env variables
skabashnyuk Mar 3, 2018
73d1cdf
fixup! fixup! Allow to configure logback logger with env variables
skabashnyuk Mar 3, 2018
58f03be
fixup! fixup! fixup! Allow to configure logback logger with env varia…
skabashnyuk Mar 3, 2018
04a6bef
fixup! fixup! fixup! fixup! Allow to configure logback logger with en…
skabashnyuk Mar 3, 2018
92a571b
fixup! fixup! fixup! fixup! fixup! Allow to configure logback logger …
skabashnyuk Mar 3, 2018
10e7091
fixup! fixup! fixup! fixup! fixup! fixup! Allow to configure logback …
skabashnyuk Mar 3, 2018
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
11 changes: 11 additions & 0 deletions assembly/assembly-ide-war/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-commons-j2ee</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-commons-logback</artifactId>
<exclusions>
<exclusion>
<artifactId>che-core-api-core</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Why is it excluded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

Why we need logback in IDE war at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@skabashnyuk skabashnyuk Mar 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why we need logback in IDE war at all?

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@skabashnyuk skabashnyuk Mar 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

<groupId>org.eclipse.che.core</groupId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>javax.servlet</groupId>
<artifactId>javax.servlet-api</artifactId>
Expand Down Expand Up @@ -112,6 +122,7 @@
<!-- dependency is required just to overlay it's content -->
<dep>org.eclipse.che:che-ide-gwt-app</dep>
<dep>org.eclipse.che.core:che-core-commons-j2ee</dep>
<dep>org.eclipse.che.core:che-core-commons-logback</dep>
<dep>ch.qos.logback:logback-classic</dep>
</ignoredUnusedDeclaredDependencies>
</configuration>
Expand Down
8 changes: 8 additions & 0 deletions core/commons/che-core-commons-logback/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@
<packaging>jar</packaging>
<name>Che Core :: Commons :: Commons Logback</name>
<dependencies>
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
</dependency>
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-core</artifactId>
</dependency>
<dependency>
<groupId>javax.inject</groupId>
<artifactId>javax.inject</artifactId>
Expand Down
99 changes: 99 additions & 0 deletions ...src/main/java/org/eclipse/che/commons/logback/EnvironmentVariablesLogLevelPropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright (c) 2012-2018 Red Hat, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Red Hat, Inc. - initial API and implementation
*/
package org.eclipse.che.commons.logback;

import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.Logger;
import ch.qos.logback.classic.LoggerContext;
import ch.qos.logback.classic.spi.LoggerContextListener;
import ch.qos.logback.core.spi.ContextAwareBase;
import ch.qos.logback.core.spi.LifeCycle;
import java.util.Arrays;

/**
* Class searches environment variable CHE_LOGGER_CONFIG Value of this variable is expected in such
* format:
*
* <p>CHE_LOGGER_CONFIG=logger1_name=logger1_level,logger2_name=logger2_level
*
* <p>In case if some logger name or logger level are omitted this pair will be silently ignored.
*/
public class EnvironmentVariablesLogLevelPropagator extends ContextAwareBase
implements LoggerContextListener, LifeCycle {

private boolean isStarted;
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@skabashnyuk skabashnyuk Mar 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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


@Override
public void start() {

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

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

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

Choose a reason for hiding this comment

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

it makes sense to trim values here

String levelStr = parts[1];

if (levelStr.isEmpty() || loggerName.isEmpty()) {
return;
}

loggerName = loggerName.trim();
levelStr = levelStr.trim();

LoggerContext lc = (LoggerContext) context;

Logger logger = lc.getLogger(loggerName);
if ("null".equalsIgnoreCase(levelStr)) {
logger.setLevel(null);
} else {
Level level = Level.toLevel(levelStr, null);
if (level != null) {
logger.setLevel(level);
}
}
}

@Override
public void stop() {
isStarted = false;
}

@Override
public boolean isStarted() {
return isStarted;
}

@Override
public boolean isResetResistant() {
return false;
}

@Override
public void onStart(LoggerContext context) {}

@Override
public void onReset(LoggerContext context) {}

@Override
public void onStop(LoggerContext context) {}

@Override
public void onLevelChange(Logger logger, Level level) {}
}
11 changes: 11 additions & 0 deletions wsagent/che-wsagent-core/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,16 @@
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-commons-lang</artifactId>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-commons-logback</artifactId>
<exclusions>
<exclusion>
<artifactId>che-core-api-core</artifactId>
<groupId>org.eclipse.che.core</groupId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.eclipse.che.core</groupId>
<artifactId>che-core-commons-schedule</artifactId>
Expand Down Expand Up @@ -162,6 +172,7 @@
<ignoredDependency>org.slf4j:jul-to-slf4j</ignoredDependency>
<ignoredDependency>org.slf4j:jcl-over-slf4j</ignoredDependency>
<ignoredDependency>org.slf4j:log4j-over-slf4j</ignoredDependency>
<ignoredDependency>org.eclipse.che.core:che-core-commons-logback</ignoredDependency>
</ignoredDependencies>
</configuration>
</execution>
Expand Down