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 to dynamically change log filename using {dynamic name} segment #230

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

simon04
Copy link
Contributor

@simon04 simon04 commented Dec 20, 2021

Description

Allow to dynamically change log filename. Say you have a task consisting of the steps foo, bar, baz, and you'd like to obtain log files foo.log, bar.log, baz.log

This PR would allow to:

Configuration.set("writer.file", "logs/{dynamic name: init}.log");
Configuration.set("writer.policies", "dynamic name");
runInit(); // logs are written to logs/init.log
DynamicNameSegment.setDynamicName("foo");
runFoo(); // logs are written to logs/foo.log
DynamicNameSegment.setDynamicName("bar");
runBar(); // logs are written to logs/bar.log
DynamicNameSegment.setDynamicName("baz");
runBaz(); // logs are written to logs/baz.log

Would you be willing to support this feature? Is this draft implementation a feasible way to accomplish the goal? Is there a more concise way?

Definition of Done

  • I read contributing.md
  • There are no TODOs left in the code
  • Code style follows the tinylog standard
  • All classes and methods have Javadoc
  • Changes are covered by JUnit tests including edge cases, errors, and exception handling
  • Maven build works including compiling, tests, and checks (mvn verify)
  • Changes are committed by a verified email address that is assigned to the GitHub account (https://github.com/settings/emails)

Documentation

Additions or amendments for the public documentation:

  • document dynamic name segment including example code from above
  • document dynamic name policy

Agreements

  • I agree that my changes will be published under the terms of the Apache License 2.0 (mandatory)
  • I agree that my GitHub user name will be published in the release notes (optional)

@pmwmedia
Copy link
Member

Thank you for your pull request, and yes, I'm willing o support this feature.

Improvement suggestions:

  • Fix the code style
  • Support an initial log name as parameter aka log file reset: my-log-file
  • LogFileResetPolicy does not need to be based on DailyPolicy (if you need both, you can configure it on runtime via daily, log file reset)
  • I'm not completely happy with the names LogFileResetPolicy and LogFileSegment, but I don't know a better name by now

@simon04
Copy link
Contributor Author

simon04 commented Dec 20, 2021

  • Fix the code style

Done.

  • Support an initial log name as parameter aka log file reset: my-log-file

Done.

  • LogFileResetPolicy does not need to be based on DailyPolicy (if you need both, you can configure it on runtime via daily, log file reset)

Very nice!

  • I'm not completely happy with the names LogFileResetPolicy and LogFileSegment, but I don't know a better name by now

Me neither. I also dislike the cross dependency between the two classes. Is there a better way? Should one class implement both interfaces?

Related (and more elaborated) concepts in other logging libraries are:

@pmwmedia pmwmedia added this to the 2.5 milestone Dec 21, 2021
@pmwmedia
Copy link
Member

You can fix the build by adding a bug pattern for ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD for org.tinylog.policies.LogFileResetPolicy into configuration/spotbugs-filters.xml.

@pmwmedia
Copy link
Member

Some thoughts and suggestions:

  • The actual log name could be stored in LogFileSegment instead of LogFileResetPolicy. This will make the LogFileResetPolicy a bit more complex, but allows to use the LogFileSegment without having a LogFileResetPolicy.
  • Currently, my favorite naming would be DynamicNameSegment and DynamicNamePolicy. However, other ideas are welcome :)
  • I have just noticed that JUnit tests for LogFileResetPolicy are missing.
  • I thought about merging the segment and policy to a single class. However, I think having separate classes are better readable and maintainable.

@simon04 simon04 force-pushed the LogFileResetPolicy branch 3 times, most recently from 503d2aa to a1392c9 Compare December 21, 2021 20:54
@codecov
Copy link

codecov bot commented Dec 21, 2021

Codecov Report

Merging #230 (b9893ed) into v2.5 (57a450b) will decrease coverage by 0.04%.
The diff coverage is 80.76%.

Impacted file tree graph

@@             Coverage Diff              @@
##               v2.5     #230      +/-   ##
============================================
- Coverage     94.59%   94.54%   -0.05%     
- Complexity     2722     2741      +19     
============================================
  Files           136      138       +2     
  Lines          5220     5246      +26     
  Branches        686      689       +3     
============================================
+ Hits           4938     4960      +22     
- Misses          154      157       +3     
- Partials        128      129       +1     
Impacted Files Coverage Δ
...main/java/org/tinylog/path/DynamicNameSegment.java 75.00% <75.00%> (ø)
...n/java/org/tinylog/policies/DynamicNamePolicy.java 83.33% <83.33%> (ø)
...pl/src/main/java/org/tinylog/path/DynamicPath.java 95.00% <100.00%> (+0.08%) ⬆️
...in/java/org/tinylog/writers/RollingFileWriter.java 82.75% <0.00%> (+0.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57a450b...b9893ed. Read the comment docs.

@pmwmedia
Copy link
Member

The release build fails because Java 6 does not support java.util.Objects.equals().

@simon04 simon04 changed the title RfC: Allow to dynamically change log filename using {logFile} segment RfC: Allow to dynamically change log filename using {dynamic name} segment Dec 21, 2021
@simon04 simon04 changed the title RfC: Allow to dynamically change log filename using {dynamic name} segment Allow to dynamically change log filename using {dynamic name} segment Dec 21, 2021
Copy link
Member

@pmwmedia pmwmedia left a comment

Choose a reason for hiding this comment

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

The code looks perfect! I found only a view Javadoc issues.

@pmwmedia
Copy link
Member

pmwmedia commented Dec 21, 2021

Thank you for your changes. It looks perfect for me :)

@simon04
Copy link
Contributor Author

simon04 commented Dec 21, 2021

Thank you for your patient and constructive reviews!

Sorry for a few glitches in Java 6 compatibility – I'm no longer used to write code targetting Java 6...

@pmwmedia pmwmedia merged commit e1b9afd into tinylog-org:v2.5 Dec 22, 2021
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

This closed pull request has been locked automatically. However, please feel free to file an issue or create a new pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants