Stable logging API - the basic use case by pgomulka · Pull Request #86612 · elastic/elasticsearch

pgomulka · 2022-05-10T14:08:43Z

Introducing a stable logging API under libs/logging.
This change covers the most common use cases for logging: fetching a logger with LogManager, emitting a log messages with Logger and Level.
It is influenced by log4j2-api, but do not include Marker and LogBuilder methods.
Also methods using org.apache.logging.log4j.util.Supplier are replaced with java.util.Supplier

The basic implementation is present in server and injected statically in LogConfigurator

relates #84478

draft

…um_use_case

elasticsearchmachine · 2022-05-23T12:52:05Z

Hi @pgomulka, I've created a changelog YAML for you.

…asticsearch into logging_api_minimum_use_case

…um_use_case

elasticmachine · 2022-05-23T13:54:53Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

pgomulka · 2022-05-23T14:11:40Z

should we mark this as experimental?

libs/logging/build.gradle

libs/logging/src/main/java/org/elasticsearch/logging/Level.java

…um_use_case

ChrisHegarty · 2022-05-24T15:46:45Z

should we mark this as experimental?

Is there a standard way of doing that ?

libs/logging/build.gradle

libs/logging/src/main/java/org/elasticsearch/logging/Level.java

server/src/main/java/org/elasticsearch/common/logging/api/impl/LoggerImpl.java

rjernst · 2022-05-24T17:05:28Z

libs/logging/src/main/java/org/elasticsearch/logging/spi/LogManagerFactory.java

+    LogManagerFactory INSTANCE = loadProvider();
+
+    static LogManagerFactory loadProvider() {
+        ServiceLoader<LogManagerFactory> sl = ServiceLoader.load(LogManagerFactory.class, ClassLoader.getSystemClassLoader());


Instead of using service loader directly here, can we have a setter method (since this package is only exported to server) and have server set this early? We won't always necessarily have this on the system classloader. @ChrisHegarty wdyt?

I was thinking that this class will change when we move the log4j off the system classloader. Something similar to what we have here https://github.com/ChrisHegarty/elasticsearch/blob/logging-module/libs/logging/src/main/java/org/elasticsearch/logging/spi/LogManagerFactory.java

How about we look up the implementation using the service loader variant that accepts a module layer? Or is the question more related to the use of service loader at all?

The latter. While eventually I think service loader will be used when we can move log4j off the system classloader, I was suggesting starting by loading this in server and setting statically here.

What is the benefit of setting this statically from server? We would have to remember to set this with LogConfigurator, and once we move implementation off the system classloader to remove it.
I personally think this is an easier to read solution, but don't have strong opinion against setting as well.

For stable plugin api, we are thinking about changing the way the system classloader is setup. Right now, everything from server is in the system classloader. For stable plugin api, we want to first load the stable api jars, then create a separate loader, parented by the stable api jars, that is everything from server. That separation would break this ServerLoader lookup, because the logging api impl would not be on the system classloader, and this code could not statically find the loader wtih the impl (since it will not yet have been created). Thus, setting this statically as an init of the logging api allows us to delay until server has actually been loaded.

libs/logging/src/main/java/org/elasticsearch/logging/spi/StandardLevels.java

…um_use_case

…gger

…um_use_case

pgomulka · 2022-05-27T11:28:57Z

@elasticmachine run elasticsearch-ci/example-plugins

…um_use_case

libs/logging/src/main/java/org/elasticsearch/logging/Level.java

ChrisHegarty · 2022-05-30T08:15:25Z

libs/logging/src/main/java/org/elasticsearch/logging/LogManager.java

+     * @return The Logger.
+     */
+    public static Logger getLogger(final String name) {
+        return LogManagerFactory.provider().getLogger(name);


I assume null in results in a NPE? If so, maybe a class-level comment, e.g.

Unless otherwise noted, passing a null argument to a method of any class will cause a NullPointerException

actually this depends on log4j. and if a null is passed as a name or clazz parameter, it will take caler's class
StackLocatorUtil.getCallerClass(2)

We should clarify this in the javadoc, no ? Or alternatively, just disallow null (throw NPE)

Has this been address in the javadoc? I'm not seeing it

I did not add the javadoc. This depends on how log4j does this and I did not want to mention this in the API's javadoc. I think is is rare that a name is null.

ChrisHegarty · 2022-05-30T08:27:31Z

libs/logging/src/main/java/org/elasticsearch/logging/LogManager.java

+import org.elasticsearch.logging.internal.spi.LogManagerFactory;
+
+/**
+ * A class used for creating loggers.


Maybe expand this a little, e.g.
"A Manager of {@code Loggers}. This class consists of factory methods for creating and retrieving Loggers."

ChrisHegarty · 2022-05-30T08:42:43Z

libs/logging/src/main/java/org/elasticsearch/logging/Logger.java

+     * @param level           the logging level
+     * @param messageSupplier A function, which when called, produces the desired log String message;
+     */
+    void log(Level level, java.util.function.Supplier<String> messageSupplier, Throwable throwable);


Suggest, highlighting the LAZY aspect. e.g.

"Logs a lazily supplied String message associated with a given throwable. If the logger is currently enabled for the specified log message level, then a message is logged that is the result produced by the given supplier function."

Note: missing at param for throwable

rjernst

This looks good overall. I have a few more nits.

I also think it would be good to generate the javadocs and see what they look like. I think some formatting is missing in several methods.

Also, this will need to be followed up by adding the jar to the release configuration.

rjernst · 2022-06-07T20:24:35Z

libs/logging/build.gradle

+tasks.named("loggerUsageCheck").configure {enabled = false }
+
+dependencies {
+  testImplementation "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"


these should all be pulled in by the test:framework dep below right?

rjernst · 2022-06-07T20:26:51Z

libs/logging/src/main/java/org/elasticsearch/logging/LogManager.java

+     * @return The Logger.
+     */
+    public static Logger getLogger(final String name) {
+        return LogManagerFactory.provider().getLogger(name);


Has this been address in the javadoc? I'm not seeing it

rjernst · 2022-06-07T20:27:20Z

libs/logging/src/main/java/org/elasticsearch/logging/Logger.java

+     * Logs a message String with the given level.
+     *
+     * @param level   the logging level
+     * @param message the message CharSequence to log.


String not CharSequence?

rjernst · 2022-06-07T20:27:46Z

libs/logging/src/main/java/org/elasticsearch/logging/Logger.java

+
+/**
+ * Main interface for logging. Most operations are done through this interface.
+ * This is interface heavily influenced by org.apache.logging.log4j2.Logger.


reversed words: is interface -> interface is

rjernst · 2022-06-07T20:30:26Z

libs/logging/src/main/java/org/elasticsearch/logging/Logger.java

+     * that is the result produced by the given supplier function.
+     * @param messageSupplier A function, which when called, produces the desired log String message;
+     */
+    void error(java.util.function.Supplier<String> messageSupplier);


Is the fully qualified name for Supplier needed?

rjernst · 2022-06-07T20:36:00Z

libs/logging/src/main/java/org/elasticsearch/logging/Logger.java

+     * @param message the message to log
+     * @param p0      parameter to the message.
+     */
+    void trace(String message, Object p0);


I wonder if we should omit the single arg variants of these methods for all levels. The reason is it can be ambiguous with the general method taking an array of args. I'm not sure if there is a performance hit for that, but we are talking about logging so I can't imagine a single implicit array creation is going to make a huge difference.

good point, I will remove them. Should we do the same for variants of this (p0, p1... p5 ) ?

Yeah I think we should. We can always add them back.

rjernst · 2022-06-07T20:36:45Z

libs/logging/src/main/java/org/elasticsearch/logging/internal/spi/LogManagerFactory.java

+import org.elasticsearch.logging.Logger;
+
+/**
+ * SPI for creating new loggers


not really SPI anymore, it's just a static holder for the factory

but doesn't it still serve a purpose of SPI? the implementation has to provide the concrete getLogger methods that this class declares

SPI normally has the connotation of being loaded by ServiceProvider, but this is not.

rjernst · 2022-06-07T20:38:14Z

server/src/main/java/org/elasticsearch/common/logging/internal/LogManagerFactoryImpl.java

+public class LogManagerFactoryImpl extends LogManagerFactory {
+    @Override
+    public Logger getLogger(String name) {
+        // TODO PG logger impl instance caching


Can you create an issue for this?

Thanks! Please add a link to the issue in the comment :)

rjernst

LGTM

rjernst · 2022-06-09T14:40:02Z

server/src/main/java/org/elasticsearch/common/logging/internal/LoggerImpl.java

+    }
+
+    private org.apache.logging.log4j.util.Supplier<?> mapSupplier(Supplier<String> msgSupplier) {
+        return (org.apache.logging.log4j.util.Supplier<Object>) () -> msgSupplier.get();


is the cast necessary given the return type?

…um_use_case

Stable logging API

2230c09

draft

pgomulka added the WIP label May 10, 2022

elasticsearchmachine added the v8.3.0 label May 10, 2022

pgomulka added 10 commits May 16, 2022 11:40

minimum use case - no message

978ebe8

javadoc and supplier<String>

93491a4

move impl to server

a5685a1

Merge remote-tracking branch 'upstream/master' into logging_api_minim…

c8caa10

…um_use_case

logmanagerfactory spi in server

f6c0e80

remove test usgae

5ce1792

spotless

3662a16

draft javadoc

a237382

tests

a783428

javadoc

28e5985

pgomulka changed the title ~~[draft] Stable logging API~~ Stable logging API - the basic use case May 23, 2022

pgomulka added :Core/Infra/Logging Log management and logging utilities >feature and removed WIP labels May 23, 2022

pgomulka self-assigned this May 23, 2022

pgomulka added 4 commits May 23, 2022 14:52

Update docs/changelog/86612.yaml

140cb19

fix javadoc

ae74deb

Merge branch 'logging_api_minimum_use_case' of github.com:pgomulka/el…

1486883

…asticsearch into logging_api_minimum_use_case

Merge remote-tracking branch 'upstream/master' into logging_api_minim…

b953db4

…um_use_case

pgomulka marked this pull request as ready for review May 23, 2022 13:54

elasticmachine added the Team:Core/Infra Meta label for core/infra team label May 23, 2022

pgomulka mentioned this pull request May 23, 2022

Lightweight Logging API #84478

Open

19 tasks

ChrisHegarty reviewed May 24, 2022

View reviewed changes

libs/logging/build.gradle Outdated Show resolved Hide resolved

ChrisHegarty reviewed May 24, 2022

View reviewed changes

libs/logging/src/main/java/org/elasticsearch/logging/Level.java Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/master' into logging_api_minim…

b5555dd

…um_use_case

rjernst reviewed May 24, 2022

View reviewed changes

pgomulka added 3 commits May 24, 2022 21:04

code review follow up

4e587ac

Merge remote-tracking branch 'upstream/master' into logging_api_minim…

d6d492b

…um_use_case

move api.impl to common.logging.internal and spi to internal.spi

657e09f

craigtaverner added v8.4.0 and removed v8.3.0 labels May 25, 2022

pgomulka added 3 commits May 27, 2022 10:21

remove standard level, statically set logManagerFactory, remove getLo…

499a2be

…gger

Merge remote-tracking branch 'upstream/master' into logging_api_minim…

a9878a3

…um_use_case

precommit

224403c

pgomulka added 3 commits May 27, 2022 15:43

Merge remote-tracking branch 'upstream/master' into logging_api_minim…

b072e8f

…um_use_case

remove module uses

4f17be8

Merge remote-tracking branch 'upstream/master' into logging_api_minim…

d5f4a2b

…um_use_case

ChrisHegarty reviewed May 30, 2022

View reviewed changes

code review followup

b8fb661

ChrisHegarty approved these changes May 31, 2022

View reviewed changes

pgomulka requested a review from rjernst May 31, 2022 14:51

rjernst reviewed Jun 7, 2022

View reviewed changes

pgomulka added 4 commits June 8, 2022 16:43

remove log methods

ee28699

small change in a javadoc for log manager factory

1d71312

rename logmanagerFactory to loggerFactory

29cc739

add link to issue in todo comment

2108c36

rjernst approved these changes Jun 9, 2022

View reviewed changes

pgomulka added 2 commits June 9, 2022 16:52

remove unnecessary cast

f902388

Merge remote-tracking branch 'upstream/master' into logging_api_minim…

1a4c58e

…um_use_case

pgomulka merged commit 0ef15b4 into elastic:master Jun 13, 2022

ChrisHegarty unassigned pgomulka Oct 29, 2024

ywangd mentioned this pull request Jan 13, 2025

[7.17] More verbose logging in IndicesSegmentsRestCancellationIT (#113844) #120025

Merged

Conversation

pgomulka commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented May 23, 2022

Uh oh!

elasticmachine commented May 23, 2022

Uh oh!

pgomulka commented May 23, 2022

Uh oh!

Uh oh!

Uh oh!

ChrisHegarty commented May 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pgomulka commented May 27, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

pgomulka commented May 10, 2022 •

edited

Loading

pgomulka May 24, 2022 •

edited

Loading

rjernst left a comment •

edited

Loading

pgomulka Jun 8, 2022 •

edited

Loading