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

Allow configuration of DEBUG logging using Eclipse bundle tracing #1366

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

guw
Copy link
Contributor

@guw guw commented May 2, 2023

Eclipse has a nice feature in its self-hosting the launch configurations which allow to enable tracing (debug output). However, bundles need to check the DebugOptions programmatically and then write to System.out. This change enhances the configuration of Logback to allow two things.

  • Enable console output when -consolelog is specified
  • Set log levels to DEBUG for any DebugOption key with a /debugLog suffix

guw added 3 commits May 1, 2023 15:12
Eclipse uses bundle tracing in the launch dialog to configure OSGi
DebugOptions. This enhancements adds support for configuring debug
logging using an key ending with '/debugLog'.
@laeubi
Copy link
Member

laeubi commented May 2, 2023

@guw I'm not quite sure this is the right approach, since you can open the MavenConsole to see the logoutput already, so instead of system.out it might should print to the usual eclipse log instead?

@laeubi laeubi requested a review from HannesWell May 2, 2023 13:17
@guw
Copy link
Contributor Author

guw commented May 3, 2023

@laeubi I'm using the M2E Logback support outside of M2E for other plug-in development. It's so much better to have SLF4J API available without worrying about DebugOptions and stuff. I originally had a separate SL4J bridge/integration available but that was conflicting with M2E. Hence my idea to make the M2E logback configuration a bit more flexible.

I agree that it's not a use case for M2E. Would it still possible to allow this in the logback config?

@HannesWell
Copy link
Contributor

HannesWell commented May 3, 2023

I agree that it's not a use case for M2E. Would it still possible to allow this in the logback config?

Only reading a system property in the logback config would be fine for me.
This would even have the advantage that m2e-users can set it in their ini without the need to alter this config (which is extracted into the workspace or configuration metadata area).
And you can set system-properties easily in launch-configs for debugging purposes.

@guw
Copy link
Contributor Author

guw commented May 3, 2023

@HannesWell With this change you wouldn't need to set the system property at all. It's only used as an "implementation detail".

Just the -consoleLog would be sufficient:
image

Debug logging can also be enabled in the UI. For example, the following would enable DEBUG level for com.salesforce.bazel.eclipse.core logger:
image

@laeubi
Copy link
Member

laeubi commented May 5, 2023

m2e uses logback only internally (or at laest is should do so) so I don't see how this can conflict?
Maintaining code that is actually never used by m2e itself is waiting for "regressions" to happen as no one knows about that "outside" functionality.

As said, everything is printed to the Maven Console View and that the purpose of the bundle, if you want it to behave different it might be better to remove the logback fragment from your product and supply your own one.

@guw
Copy link
Contributor Author

guw commented May 5, 2023

m2e uses logback only internally (or at laest is should do so) so I don't see how this can conflict?

It's the configuration that conflicts. Requiring other plug-in developers to ship their own configurator will lead to a situation where M2E's log output can become disabled. Thus, having the ability to plug into M2E Logback configuration allows to mitigate the situation.

The use of Logback is not exclusive to M2E. It's a logging system anyone can use (together with SLF4J API).

Maintaining code that is actually never used by m2e itself is waiting for "regressions" to happen as no one knows about that "outside" functionality.

Agreed. Question remains whether configuration of log levels via Eclipse Tracing tab is something M2E developers find useful.

it might be better to remove the logback fragment from your product and supply your own one

That's not how Eclipse plug-ins and marketplace works. Eclipse users are allowed to install things freely from the marketplace. I am trying to help plug-ins work nicely together for a smoother experience.

@laeubi
Copy link
Member

laeubi commented May 7, 2023

I don't really get what you want to archive. The logback used by m2e is exclusively for m2e is it not meant to be shared / reused by others. This is because m2e has an embedded maven what uses slf4j as an implementation detail, as it is an implementation detail of m2e that is uses logback as its (internal) backend.

So for the default case (Eclipse) there is the maven console view you can open and see the output and you don't need to configure anything. If you don't want that (e.g. in a custom product / library) you can supply an own fragment and replace what m2e does.

Beside that, binding anything to -consoleLog to print to system.out is simply wrong, as it has a completely different semantics as it logs OSGi Log(!) entries to the console, so if you want that, the way would be to not use logback but slf4j-osgi, that will redirect everything to the OSGi Logg system what then will print everything out with -consoleLog, especially as the "console" is not necessarily the System.out (it can be a telnet or whatever else...).

@guw
Copy link
Contributor Author

guw commented May 7, 2023

Oh I am not targeting M2E users. I've added this as a helper for plug-in developers. Anyway, it seems that your definition of "default Eclipse" is very narrow and excludes the plug-in ecosystem. In such a broader ecosystem Logback/SLF4J is only an implementation detail of M2E until a users installs another plug-in which brings its own Logback Configurator implementation along. It's ok if that doesn't seem to bother M2E and the PR is not needed.

@HannesWell
Copy link
Contributor

HannesWell commented May 7, 2023

Oh I am not targeting M2E users.

From what you explained just providing the system property seems to be a good compromise to me. It gives you a point to hook into and the code can be added to your Plugin where it is really used.

@laeubi
Copy link
Member

laeubi commented May 8, 2023

I'm sorry but its still not clear what type of plugins you are referring to?

org.eclipse.m2e.logback is an optional development feature it is not installed by default and it serves a very special purpose. So usually a user can not install "another plug-in which brings its own Logback Configurator" as this can not work by design and the user has to choose what of both choices is the best for their usecase.

So if you want to use the logback backend for general purpose for general purpose plugins you should not use org.eclipse.m2e.logback (but will loose the console feature). If you still want to use the features, you can use it and @HannesWell said you can modify the logback config as it is extracted as part of first initialization but you wont be able to use another plugin that configures logback, what is true vice versa.

So to summarize:

  1. org.eclipse.m2e.logback is not meant as a generic general purpose way to provide an SLF4J backend to eclipse
  2. Configuring m2e itself with DebugOptions is great
  3. using -consolelog to print to System.out is not really a good way but we might want to have a debug option that has an appender that redirect to the Eclipse Log (and then they will appear if you specify -consolelog.

@HannesWell
Copy link
Contributor

@guw I wonder if it wouldn't actually be better for your case if the m2e-logback configuration would be modular and would be applied on top of an existing application configuration (if present). Logback can also be configured dynamically in code and I could check if that would be possible as intended.
If no configuration is supplied a equivalent one would be created (via code) and if one is already present the changes for the Maven-Console would be applied, with as few changes as possible.
Would that help four your use-case? Then you can just configure logback the 'classical' way.

@guw
Copy link
Contributor Author

guw commented May 10, 2023

Would that help four your use-case? Then you can just configure logback the 'classical' way.

I need two things:

  • ability to see log output from my SLF4J loggers in console when using -consoleLog
  • ability to enable (assuming default is OFF) DEBUG level for a logger (or plug-in)

I am not sure if the dynamic Logback configuration would allow the same capabilities of what M2E needs. M2E's default configuration is pretty good IMO.

@laeubi
Copy link
Member

laeubi commented May 10, 2023

I need two things:

* ability to see log output from my SLF4J loggers in console when using `-consoleLog`

* ability to enable (assuming default is OFF) `DEBUG` level for a logger (or plug-in)

But what kind of plugin is it? A Maven-Plugin? Then m2e-logback would be the right choice. If not m2e-logback is simply they wrong place, m2e does not offer generic slf4j binding to the eclipse platform as we can't afford managing all the complications (where some you already have mentioned here), in such a case you should ask for generic logging support (with whatever back-end) at the eclipse.platform project, e.g. there was a discussion already here:

@HannesWell
Copy link
Contributor

Would that help four your use-case? Then you can just configure logback the 'classical' way.

I need two things:

* ability to see log output from my SLF4J loggers in console when using `-consoleLog`

* ability to enable (assuming default is OFF) `DEBUG` level for a logger (or plug-in)

I am not sure if the dynamic Logback configuration would allow the same capabilities of what M2E needs. M2E's default configuration is pretty good IMO.

Sorry for the late reply, but I was busy with other tasks.

Since I'm about to perform a release I submitted your commit that only changes m2e's logback configuration (your second point) in #1399. This will allow you to apply the other changes of this PR in your plugin to achieve the desired goals.
As already said m2e-logback is not intended as general purpose logging-backend for Eclipse (but we cannot prevent you or others from using it as such). Nevertheless changing the m2e.logback Plugin to apply the configuration dynamically would actually be better for product builders, because they can then configure logback as they desire the usual way and m2e just sits on top of that.
In general the logging-backend should always be configured by those that build the final product, never by libraries. And if we change m2e.logback accordingly you can do that in your product and of course just copy the configuration of M2E :)

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