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.

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

Improve tracing capability of m2e through m2e.logback.configuration. #1589

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

rgrunber
Copy link
Contributor

@rgrunber rgrunber commented Nov 4, 2020

  • Add 'org.eclipse.m2e.logback.feature' to target platform
  • Add 'org.eclipse.m2e.logback' plugin to language server product
  • This exposes the 'logback.configurationFile' system property through
    which one can provide a logback configuration

Signed-off-by: Roland Grunberg rgrunber@redhat.com

A few notes on how this could work :

Suppose we want to log using standard out/err :

In : /path/to/logback.xml

<configuration scan="true">
  <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
    <encoder class="ch.qos.logback.classic.encoder.PatternLayoutEncoder">
      <pattern>%date [%thread] %-5level %logger{35} - %msg%n</pattern>
    </encoder>
  </appender>
  <root level="DEBUG">
    <appender-ref ref="STDOUT" />
  </root>
</configuration>

Now when launching JDT-LS, we need to set -Dlogback.configurationFile=/path/to/logback.xml -Djdt.ls.debug=true
(The latter ensures JDT-LS will create the correct log files in the workspace .metadata folder. One would need to use java.jdt.ls.vmargs to set these on the client side.

A custom logfile location is also possible, but all that configuration would happen in the logback.xml without the need for setting
jdt.ls.debug=true

Update :

With this change, the default logging for m2e would generate 'INFO' level logging only in ${workspace}/.metadata/.plugins/org.eclipse.m2e.logback.configuration/0.log.

@fbricon

@fbricon fbricon requested a review from snjeza November 5, 2020 16:31
Copy link
Contributor

@snjeza snjeza left a comment

Choose a reason for hiding this comment

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

@rgrunber
Copy link
Contributor Author

rgrunber commented Nov 6, 2020

@snjeza snjeza self-requested a review November 6, 2020 00:51
@testforstephen
Copy link
Contributor

Would you consider sending a PR at vscode-java side to adopt the new log capability?

@rgrunber
Copy link
Contributor Author

rgrunber commented Nov 6, 2020

I think this would be the next logical step. Making it easier to do the above steps by enabling some preference.

@testforstephen
Copy link
Contributor

OK, i'm saying this to just want a bits to have a look at the whole E2E workflow in VS Code.

A few questions here:

  • Will the logs from the new m2e.logback be merged to the existing jdt.ls .log file under .metadata directory?
  • Is this enabled by default, right?
    My understanding is that the -Dlogback.configurationFile=/path/to/logback.xml vmArgs is just an optional parameter. It's used to override the log preference such as the log level. If you don't specify this parameter, m2e.logback will startup with the default preference, right?

@rgrunber
Copy link
Contributor Author

rgrunber commented Nov 6, 2020

* Will the logs from the new m2e.logback be merged to the existing jdt.ls .log file under .metadata directory?

No, they will be merged into .out-jdt.ls-${date}.log which would be right beside the .log file. The jdt.ls.debug=true creates new logfiles that this change takes advantage of.

If you want to log directly to The Eclipse workspace .log, I believe this should be possible, though I haven't tested. There is an additional plugin, org.eclipse.m2e.logback.appender (also fairly small) that provides EclipseLogAppender

* Is this enabled by default, right?

In the process of answering this, I've found a possible issue. While the above isn't enable by default, there is some logging that can happen by default. By just adding the configuration plugin, and if no logback file is defined, it defaults to the logback.xml file stored within the plugin. STDOUT/EclipseLog/MavenConsoleLog won't do anything by default, but FILE will log because ${org.eclipse.m2e.log.dir} is set to the workspace location by default. It's set to INFO though, so it's not verbose.

  My understanding is that the `-Dlogback.configurationFile=/path/to/logback.xml` vmArgs is just an optional parameter. It's used to override the log preference such as the log level. If you don't specify this parameter, m2e.logback will startup with the default preference, right?

Yes. Without that property, m2e will use the default logback settings, which it generates in ${workspace}/.metadata/.plugins/org.eclipse.m2e.logback.configuration/logback.xml

@snjeza
Copy link
Contributor

snjeza commented Nov 6, 2020

There is an additional plugin, org.eclipse.m2e.logback.appender (also fairly small) that provides EclipseLogAppender

The org.eclipse.m2e.logback.appender plugin requires the org.eclipse.m2e.core.ui and org.eclipse.ui.* plugins. We can't use it in Java LS.

@rgrunber
Copy link
Contributor Author

rgrunber commented Nov 6, 2020

There is an additional plugin, org.eclipse.m2e.logback.appender (also fairly small) that provides EclipseLogAppender

The org.eclipse.m2e.logback.appender plugin requires the org.eclipse.m2e.core.ui and org.eclipse.ui.* plugins. We can't use it in Java LS.

Yup, Looks like because of the Maven appender relying on some m2e ui components.

So the only remaining concern is about the new INFO-level logging in .metadata/.plugins/org.eclipse.m2e.logback.configuration/0.log that this would introduce. I think the amount is comparable to what logging is done in .metadata/.log .

@testforstephen
Copy link
Contributor

I see.

Since this new trace capability is just used to diagnose the language server itself, rather than the user's application, and asking the end user to provide a logback.xml file to enable verbose looks a little too heavy.

A more reasonable approach is only exposing one switch jdt.ls.debug to control the verbose mode for jdt.ls. The extension could auto feed the corresponding logback.xml configurations to logback plugin based on jdt.ls.debug. For example, when the extension detects the jdt.ls.debug is enabled, then automatically add the built-in verbose config to the vmArgs -Dlogback.configurationFile=/extension_internal_path/logback.xml. Also this could be done at eclipse.jdt.ls side if it supports setting logback.configurationFile via programming API.

@rgrunber
Copy link
Contributor Author

rgrunber commented Nov 9, 2020

Now, setting jdt.ls.debug=true should automatically enable DEBUG level logging (provided by our own logback.xml) into ${workspace}/.metadata/.out-jdt.ls-${timestamp}.log as long as logback.configurationFile has not been set.

The requirement (in the code) that logback.configurationFile not be set is so that users could still have jdt.ls.debug=true but provide their own logback xml file.

- Add 'org.eclipse.m2e.logback.feature' to target platform
- Add 'org.eclipse.m2e.logback' plugin to language server product
- When system property 'jdt.ls.debug=true' is set (eg. by
  java.jdt.ls.vmargs), and 'logback.configurationFile' is not set ,
  verbose (DEBUG) logging will be done on the standard output of JDT-LS.
- The system property 'logback.configurationFile' is also available for
  advanced debugging usages

Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
@snjeza
Copy link
Contributor

snjeza commented Nov 10, 2020

test this please

@rgrunber
Copy link
Contributor Author

test this please

I've tested the change with/without jdt.ls.debug and it looks like it's working for me. I've also tested the launch configurations for debugging in VSCode and they also appear to start up and connect.

@snjeza
Copy link
Contributor

snjeza commented Nov 10, 2020

test this please
I've tested the change with/without jdt.ls.debug and it looks like it's working for me. I've also tested the launch configurations for debugging in VSCode and they also appear to start up and connect.

"test this please" is a github command. It restarts the build.

@snjeza
Copy link
Contributor

snjeza commented Nov 10, 2020

test this please

@testforstephen testforstephen added this to the Mid November 2020 milestone Nov 11, 2020
@testforstephen testforstephen merged commit 9f116c2 into eclipse-jdtls:master Nov 11, 2020
@rgrunber rgrunber deleted the m2e-tracing branch November 12, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants