Introduce logger REST API to manage log level at runtime #9161

benoitf · 2018-03-21T08:52:20Z

What does this PR do?

following this previous comment #8997 (comment)

with this Rest API, it's possible to create new logger with log level, list all loggers defined with loglevel, change level on a given logger, etc

On multi-user, System#MANAGE_SYSTEM permission is required.

Change-Id: I1c105aca33cc88f90270ade4d792d3a75191740a
Signed-off-by: Florent BENOIT fbenoit@redhat.com

What issues does this PR fix or reference?

#8997

It's possible to create new logger with log level, list all loggers defined with loglevel, change level on a given logger, etc Change-Id: I1c105aca33cc88f90270ade4d792d3a75191740a Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

skabashnyuk · 2018-03-21T08:55:29Z

wsmaster/che-core-api-logger/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>org.eclipse.che.core</groupId>
+            <artifactId>che-core-db</artifactId>


why it's provided?

copied from https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-workspace/pom.xml#L139

removed the dependency

skabashnyuk · 2018-03-21T08:55:54Z

wsmaster/che-core-api-logger/pom.xml

+        <dependency>
+            <groupId>com.google.inject.extensions</groupId>
+            <artifactId>guice-persist</artifactId>
+            <scope>provided</scope>


why its provided?

copied from https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-workspace/pom.xml#L119

removed the dependency

skabashnyuk · 2018-03-21T08:56:28Z

wsmaster/che-core-api-logger/src/main/java/org/eclipse/che/api/logger/LoggerService.java

+ * @author Florent Benoit
+ */
+@Api(value = "/logger", description = "Logger REST API")
+@Path("/logger")


can you make sure this service is protected with authentification in multi-user mode?

for now it was not enabled in multi user mode

permission mechanism added

skabashnyuk · 2018-03-21T08:57:13Z

wsmaster/che-core-api-logger/src/main/java/org/eclipse/che/api/logger/LoggerService.java

+  @ApiResponses({
+    @ApiResponse(code = 200, message = "The loggers successfully fetched"),
+  })
+  public List<LoggerDto> getLoggers() {


should we add pagination here? with limit 30 by default

We could yes but for now there is only 3 loggers
https://github.com/eclipse/che-lib/blob/master/che-tomcat8-slf4j-logback/src/assembly/conf/logback.xml#L47-L49

added pagination

sleshchenko

Cool and useful feature 👍
But It looks like authorization is missed here. I guess It's not OK to this API should is available for all authenticated users. Or I missed something?

sleshchenko · 2018-03-21T08:56:48Z

wsmaster/che-core-api-logger/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>org.eclipse.che.core</groupId>
+            <artifactId>che-core-db-vendor-h2</artifactId>


Please revise dependencies list, I guess you don't need che-core-db-vendor-h2, org.eclipse.persistence.jpa dependencies, do you?

yes I need to cleanup

sleshchenko · 2018-03-21T08:58:32Z

wsmaster/che-core-api-logger/src/main/java/org/eclipse/che/api/logger/LoggerService.java

+  @ApiResponses({
+    @ApiResponse(code = 200, message = "The logger successfully created"),
+  })
+  public LoggerDto createLogger(


I think this method should be covered with System#MANAGE_SYSTEM permission actions. Or maybe even all methods of this service. WDYT?

permission mechanism added

benoitf · 2018-03-21T09:21:58Z

I should have specified that for now there is now permission check on this service, mainly because I added only this service on the single user and not the multi user.

https://github.com/eclipse/che/blob/fc3fd1dc83ba7d7305e54c956353ebdae7b2aedb/assembly/assembly-wsmaster-war/src/main/java/org/eclipse/che/api/deploy/WsMasterModule.java#L249-L269

But yes I wanted to include it as well for multi-user, just wanted to discuss on how you were seeing the permission that would be required to call it. It seems System#MANAGE_SYSTEM is a choice ?

Change-Id: I626a03fde2ea2ecc65bee50dbae9f4135b46fdcc Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

skabashnyuk · 2018-03-21T09:30:33Z

It seems System#MANAGE_SYSTEM is a choice

I think yes.

Change-Id: I4596f22ae6f28a9317d147b4f29bb090ddb7e9c6 Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

benoitf · 2018-03-21T10:54:41Z

ok I have added permission stuff and then enabled as well logger api for multi user mode

Change-Id: I59a1136c450103dec23bf4b1b5781a9fbeea1b91 Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

Change-Id: I56aa8df29c8ee3ab876c97812b6a139854914a0f Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

sleshchenko

LGTM

sleshchenko · 2018-03-21T11:12:23Z

wsmaster/che-core-api-logger-shared/pom.xml

+                            </dtoPackages>
+                            <outputDirectory>${dto-generator-out-directory}</outputDirectory>
+                            <genClassName>org.eclipse.che.api.logger.server.dto.DtoServerImpls</genClassName>
+                            <impl>server</impl>


I'd say that it is better to generate DTO server implementations in server module. Because a shared module is supposed to contain only shared things which are compatible with dependent modules.
So GWT module will get classes that can't be used there.
WDYT? Does it make sense?

I copied content from workspace-shared module so maybe if I'm doing it wrong there is an issue with this module as well ?

it's up to you, let me know what is the better approach.
maybe client or server should be additional classifier artifacts

main artifacts = interfaces
client classifier = for gwt
server classifier = for server

I like the idea with classifiers.

Is an issue with this module as well

I think yes.

I think now we can leave it as it is. And later rework it in all places.
@skabashnyuk WDYT?

I think we can revise it later.

riuvshin · 2018-03-21T12:12:54Z

wsmaster/che-core-api-logger-shared/pom.xml

+            <plugin>
+                <groupId>org.eclipse.che.core</groupId>
+                <artifactId>che-core-api-dto-maven-plugin</artifactId>
+                <version>${project.version}</version>


should there ${che.version} ?

@riuvshin
it's the same than here
https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-user-shared/pom.xml#L54
or here https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-workspace-shared/pom.xml#L59

what should I do ?

ah, ok, in theory both are fine, but I thought we use che.version in such places so it seems it is ok

benoitf added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Mar 21, 2018

benoitf self-assigned this Mar 21, 2018

benoitf requested review from skabashnyuk, mshaposhnik and sleshchenko March 21, 2018 08:52

benoitf requested review from riuvshin and vparfonov as code owners March 21, 2018 08:52

skabashnyuk reviewed Mar 21, 2018

View reviewed changes

sleshchenko reviewed Mar 21, 2018

View reviewed changes

fixup! Introduce logger REST API to manage log level at runtime

ac6eab8

Change-Id: I626a03fde2ea2ecc65bee50dbae9f4135b46fdcc Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

fixup! Introduce logger REST API to manage log level at runtime

ff97d51

Change-Id: I4596f22ae6f28a9317d147b4f29bb090ddb7e9c6 Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

benoitf added 2 commits March 21, 2018 11:56

fixup! Introduce logger REST API to manage log level at runtime

3871773

Change-Id: I59a1136c450103dec23bf4b1b5781a9fbeea1b91 Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

fixup! Introduce logger REST API to manage log level at runtime

7152262

Change-Id: I56aa8df29c8ee3ab876c97812b6a139854914a0f Signed-off-by: Florent BENOIT <fbenoit@redhat.com>

sleshchenko approved these changes Mar 21, 2018

View reviewed changes

skabashnyuk approved these changes Mar 21, 2018

View reviewed changes

riuvshin reviewed Mar 21, 2018

View reviewed changes

mshaposhnik approved these changes Mar 21, 2018

View reviewed changes

riuvshin approved these changes Mar 21, 2018

View reviewed changes

vparfonov approved these changes Mar 21, 2018

View reviewed changes

benoitf merged commit f051b7f into master Mar 21, 2018

benoitf deleted the logger-rest branch March 21, 2018 14:17

benoitf added this to the 6.3.0 milestone Mar 21, 2018

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 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce logger REST API to manage log level at runtime #9161

Introduce logger REST API to manage log level at runtime #9161

benoitf commented Mar 21, 2018 •

edited

Loading

skabashnyuk Mar 21, 2018

benoitf Mar 21, 2018

benoitf Mar 21, 2018

skabashnyuk Mar 21, 2018

benoitf Mar 21, 2018

benoitf Mar 21, 2018

skabashnyuk Mar 21, 2018

benoitf Mar 21, 2018

benoitf Mar 21, 2018

skabashnyuk Mar 21, 2018 •

edited

Loading

benoitf Mar 21, 2018

benoitf Mar 21, 2018

sleshchenko left a comment

sleshchenko Mar 21, 2018

benoitf Mar 21, 2018

benoitf Mar 21, 2018

sleshchenko Mar 21, 2018

benoitf Mar 21, 2018

benoitf commented Mar 21, 2018

skabashnyuk commented Mar 21, 2018

benoitf commented Mar 21, 2018

sleshchenko left a comment

sleshchenko Mar 21, 2018

benoitf Mar 21, 2018

sleshchenko Mar 21, 2018

skabashnyuk Mar 21, 2018

riuvshin Mar 21, 2018

benoitf Mar 21, 2018

riuvshin Mar 21, 2018

Introduce logger REST API to manage log level at runtime #9161

Introduce logger REST API to manage log level at runtime #9161

Conversation

benoitf commented Mar 21, 2018 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

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 21, 2018 • edited Loading

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

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

benoitf commented Mar 21, 2018

skabashnyuk commented Mar 21, 2018

benoitf commented Mar 21, 2018

sleshchenko left a 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

benoitf commented Mar 21, 2018 •

edited

Loading

skabashnyuk Mar 21, 2018 •

edited

Loading