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

Add logback as logging framework #19

Merged
merged 4 commits into from
Apr 20, 2023
Merged

Add logback as logging framework #19

merged 4 commits into from
Apr 20, 2023

Conversation

stefanhahmann
Copy link
Collaborator

This Pull Request adds logback as a logging framework that may be used to print debug information at different levels and also to log files.
Log levels TRACE, DEBUG and INFO are forwarded to System.out
Log levels WARN and ERROR are forwarded to System.err.

Copy link
Collaborator

@maarzt maarzt left a comment

Choose a reason for hiding this comment

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

I can't really comment on all the configuration of logback as I don't have any experience there.

Good news for you. I found that logback and slf4j are already part of the Fiji installation. (I simply searched for the jars in my Fiji installation.) So using it here should be fine. Not sure of what will happen if this project becomes part of the official mastodon, or Fiji.

@tinevez What do you think?

@maarzt maarzt closed this Mar 30, 2023
@maarzt maarzt reopened this Mar 30, 2023
@maarzt
Copy link
Collaborator

maarzt commented Mar 30, 2023

Sorry for closing the PR, that was an accident.

@tinevez
Copy link
Collaborator

tinevez commented Mar 30, 2023

Like Mathias my expertise is limited.
At one point I dreamt of having a central logging windows for Mastodon, based on Mathias LogPanel. I think I created an issue and a PR with that. Let me check

Here!
mastodon-sc/mastodon#58

It will need to be rewritten of course, but could be a nice item for the beta-29. It would offer a service for all Mastodon plugins that you could subscribe to have progress bar and messages to the user.

@stefanhahmann
Copy link
Collaborator Author

Like Mathias my expertise is limited. At one point I dreamt of having a central logging windows for Mastodon, based on Mathias LogPanel. I think I created an issue and a PR with that. Let me check

Here! mastodon-sc/mastodon#58

It will need to be rewritten of course, but could be a nice item for the beta-29. It would offer a service for all Mastodon plugins that you could subscribe to have progress bar and messages to the user.

Thanks for pointing me to this approach. I like the LogPanel and I also like the ProgressBar! Nevertheless, I think, there is some benefit in combining this with logging framework. The logging framework provides some features out of the box:

  1. More fine-grained log levels than only INFO and ERROR, which may be handy when developing/debugging.
  2. Logging to files, which may be handy, when the user closed Fiji/Mastodon, but still wants to report a bug.
  3. Logging format can controlled/adapted to special needs via the configuration file.
  4. Logging statement do not need to be concatenated via String operations.

There is the possibility of implementing a so called "Custom Appender" (cf. e.g. https://www.baeldung.com/custom-logback-appender) that could forward logging statements to the LogPanel and by this both approaches could be combined.

@stefanhahmann
Copy link
Collaborator Author

I can't really comment on all the configuration of logback as I don't have any experience there.

Good news for you. I found that logback and slf4j are already part of the Fiji installation. (I simply searched for the jars in my Fiji installation.) So using it here should be fine. Not sure of what will happen if this project becomes part of the official mastodon, or Fiji.

@tinevez What do you think?

@maarzt thanks for pointing out that logback and slf4j are already part of the Fiji installation. This means that I should not include any version numbers for these artefacts and would use the versions that come with the parent artefact.

@stefanhahmann
Copy link
Collaborator Author

@maarzt I requested a re-review, not in terms of code actually, but to see, if the conversations can be resolved so that the PR could be merged.

Copy link
Collaborator

@maarzt maarzt left a comment

Choose a reason for hiding this comment

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

I though about this a little bit more:

  • Logback is in fact not a compile time dependency of your code. SLF4J is the only dependency. You could declare logback as a "runtime", "test" or "optional" dependency in the maven pom. This might give a bit more freedom when putting this jar on an update site.
  • The src/main/resources/logback.xml will be part of the compiled jar. To me this feels as if this configuration could override or cause a conflict the global logback configuration in FIJI. Maybe this is worth checking. Is "src/main/resource/logback.xml" actually needed?
  • Maybe the best way is to have logback as a test-runtime dependency, and not messing with the FIJIs global logback configuration in the repo.

@stefanhahmann
Copy link
Collaborator Author

I though about this a little bit more:

* Logback is in fact not a compile time dependency of your code. SLF4J is the only dependency. You could declare logback as a "runtime", "test" or "optional" dependency in the maven pom. This might give a bit more freedom when putting this jar on an update site.

Thanks for this advice. I also read about logging in Fiji on https://imagej.net/develop/logging and your advice is perfectly in line what is stated there. I have changed the scope of the logback dependencies to <scope>test</scope>

* The src/main/resources/logback.xml will be part of the compiled jar. To me this feels as if this configuration could override or cause a conflict the global logback configuration in FIJI. Maybe this is worth checking. Is "src/main/resource/logback.xml" actually needed?

You are right. This is not the correct place for custom logback.xml. https://imagej.net/develop/logging states that a modified version of https://github.com/scijava/scijava-config should be delivered via the update sites instead. I deleted this file.

* Maybe the best way is to have logback as a test-runtime dependency, and not messing with the FIJIs global logback configuration in the repo.

Fully agreed.

* Provide logback-core and logback-classic as test-runtime dependencies
* Provide a logback-test.xml with project specific logging requirements
@sonarcloud
Copy link

sonarcloud bot commented Apr 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@stefanhahmann stefanhahmann merged commit 961b191 into master Apr 20, 2023
@stefanhahmann stefanhahmann deleted the add-logback branch April 20, 2023 12:36
@stefanhahmann stefanhahmann self-assigned this Jan 24, 2024
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