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

Revamp AbstractLogger in log4j-kit #2291

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

ppkarwasz
Copy link
Contributor

In order for Log4j Core 3.x not to depend on the utility classes in Log4j API 2.x, we provide a new AbstractLogger in log4j-sdk.

The main improvements to this AbstractLogger are:

  • The methods are grouped by functionality: public API for users, location-aware method for system integrators, "create Message and log" methods and filter methods. The most important methods are at the beginning.
  • All the dependencies of the class (message factory, flow message factory, recycler factory) must be explicitly provided. Only the log builder implementation is hardcoded.
  • The logMessageSafely message has an additional StackTraceElement parameter and all the logging methods are routed through it,
  • All the methods are tested for their bytecode length so that most of them are below 35 bytes. The exceptions are listed in AbstractLoggerTest. It is basically impossible for unrolled vararg methods with more than 2 parameters and methods that use supplier to follow a 100% inline-able path.
  • The class is annotated with JSpecify.
  • The class has only 2 unimplemented methods: doLog (formerly log) and isEnabled(Level, Marker).

Part of #2290

@ppkarwasz ppkarwasz added the STF-Milestones Milestones funded by the Sovereign Tech Fund label Feb 15, 2024
@ppkarwasz ppkarwasz added this to the 3.0.0 milestone Feb 15, 2024
@ppkarwasz ppkarwasz requested a review from vy February 15, 2024 16:03
@ppkarwasz ppkarwasz self-assigned this Feb 15, 2024
@ppkarwasz ppkarwasz marked this pull request as draft February 15, 2024 16:59
@garydgregory
Copy link
Member

garydgregory commented Feb 15, 2024

"SDK" implies to me something big, not an API, while "SPI" implies something like a template to be implemented or classes to be extended.

@ppkarwasz ppkarwasz force-pushed the fix/move-abstractlogger branch from 059f3ae to 2eab1a5 Compare February 15, 2024 19:44
@ppkarwasz
Copy link
Contributor Author

"SDK" implies to me something big, not an API, while "SPI" implies something like a template to be implemented or classes to be extended.

I don't like calling things SDK either, but I like a simple rule to determine the main package name from the artifact name. o.a.l.l.spi is already taken, what about log4j-spi3 and o.a.l.l.spi3?

@ppkarwasz ppkarwasz force-pushed the fix/move-abstractlogger branch from 2eab1a5 to f922379 Compare February 15, 2024 20:03
@vy
Copy link
Member

vy commented Feb 15, 2024

Please no enumerated naming, e.g., log4j-spi3.

I am fine with log4j-sdk. Other alternatives I can think of:

  • log4j-runtime
  • log4j-backend
  • log4j-kit
  • log4j-base

@ppkarwasz ppkarwasz force-pushed the fix/move-abstractlogger branch 3 times, most recently from 8db1575 to ba4b732 Compare February 18, 2024 00:36
@ppkarwasz ppkarwasz marked this pull request as ready for review February 19, 2024 15:51
@ppkarwasz
Copy link
Contributor Author

@garydgregory,

What name would you propose?

Regardless of the naming of the artifact can you review the AbstractLogger class? There are some new methods like logEnterMessage, logCatchingMessage and so on. The most important ones should be at the top.

Since this PR targets the feature branch feature/log4j-sdk and not main, we can solve the naming problem just before merging the feature branch into main.

In order for Log4j Core 3.x not to depend on the utility classes in
Log4j API 2.x, we provide a new `AbstractLogger` in `log4j-sdk`.
After some minor corrections flow message methods are also immediately
inlineable.
@ppkarwasz ppkarwasz force-pushed the fix/move-abstractlogger branch from 2dc9115 to 1138990 Compare February 27, 2024 09:20
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

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

This change is massive; maybe not in size, but its implications. I am not able to see anything fishy. But I guess we will iron out most wrinkles along the way.

@ppkarwasz
Copy link
Contributor Author

@vy and @garydgregory,

I renamed the module to log4j-kit. Is it OK with you?

@ppkarwasz ppkarwasz force-pushed the fix/move-abstractlogger branch from c0e8ff5 to d31e9e4 Compare February 28, 2024 11:01
@ppkarwasz ppkarwasz merged commit 1b510c2 into feature/log4j-sdk Feb 29, 2024
4 of 5 checks passed
@ppkarwasz ppkarwasz deleted the fix/move-abstractlogger branch February 29, 2024 15:14
@ppkarwasz ppkarwasz changed the title Revamp AbstractLogger in log4j-sdk Revamp AbstractLogger in log4j-kit Mar 26, 2024
@ppkarwasz ppkarwasz mentioned this pull request Mar 28, 2024
8 tasks
@vy vy removed the STF-Milestones Milestones funded by the Sovereign Tech Fund label May 31, 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