SLF4J Logger rewrite part2 #492

kantenkugel · 2017-10-01T09:11:56Z

Completely removed SimpleLog in favor of a SLF4J Logger.

Now using the Logger from slf4j-simple as fallback.
new class JDALogger acts as JDA's Logger factory. It either returns the Logger via LoggerFactory if slf4j binding is present or creates a SimpleLogger instance (and cachinges it).
Replaced all String concatenations of loggers with the lazy construction using parametrized logger methods and {}
Added JDALogger#getLazyString(LAMBDATERM) to lazily construct String from expression as slf4j doesn't support that natively yet

…meters

kantenkugel · 2017-10-01T09:12:44Z

Would suggest waiting with merge until next version bump

schnapster · 2017-10-04T11:37:22Z

May I suggest to take advantage of slf4js methods and instead of separately logging stack traces, append them to the message they belong to?

The output should look exactly the same in terms of information content, with the benefit of making life easier on log aggregators which may get confused by the current behaviour of doing two log calls for a single issue (concrete example of when things go wrong: using the logback appender for sentry)

Example:
https://github.com/DV8FromTheWorld/JDA/blob/30174cfa66e1952d1e98b7ddc5eb7803f74c535f/src/main/java/net/dv8tion/jda/core/requests/WebSocketClient.java#L975-L985

Instead do:

        catch (JSONException ex)
        {
            LOG.warn("Got an unexpected Json-parse error. Please redirect following message to the devs:\n\t{}\n\t{} -> {}",
                ex.getMessage(), type, content, ex);
        }
        catch (Exception ex)
        {
            LOG.error("Got an unexpected error. Please redirect following message to the devs:\n\t{} -> {}", type, content, ex);
        }

Shredder121 · 2017-10-04T13:53:30Z

The only issue is that if you use the Throwable method, the format arguments cannot be used.

kantenkugel · 2017-10-04T13:55:48Z

And that (together with the fact that i want the json data seperated) is why i did it the way i did

schnapster · 2017-10-04T14:00:56Z

The only issue is that if you use the Throwable method, the format arguments cannot be used.

That's not true.
https://www.slf4j.org/faq.html#paramException

schnapster · 2017-10-04T14:02:38Z

the fact that i want the json data seperated

What are the benefits of doing that? There is no information lost by attaching the stacktrace to the source of it.

Shredder121 · 2017-10-04T14:03:19Z

~~If you want it separated-but-together I can also recommend MDC to bind variables. (which you can then immediately remove if needed)~~
~~Or use String.format before it.~~
~~You can do a lot if you put everything in an if (logger.is(Warn/Error)Enabled()) statement.~~
~~I do vote in favor of keeping the calls together, as "just" the exception logged is a shame for log aggregation tools, or continuity when viewing log files~~
WOAH TIL! Thanks.
(keeping draft for posterity)

MinnDevelopment · 2017-10-04T14:06:45Z

src/main/java/net/dv8tion/jda/core/audio/AudioConnection.java

@@ -514,7 +515,7 @@ else if (sample < Short.MIN_VALUE)
                }
                catch (Exception e)
                {
-                    LOG.fatal(e);
+                    LOG.error("Something happened!", e);


Can we make the comments in this file a bit more descriptive like "Unexpected Exception occurred while X"

:) Yea i didn't know what exactly happened there

Its funny how you didn't mention the other 2 exceptions above :)

I used the plural "comments" for a reason 👀

MinnDevelopment · 2017-10-04T14:08:25Z

src/main/java/net/dv8tion/jda/core/entities/impl/GuildImpl.java

@@ -313,6 +313,7 @@ public Role getPublicRole()
    }

    @Override
+    @Deprecated
    public TextChannel getPublicChannel()


Since this is going to be in 3.4 we will be removing this method anyway FYI

just added it because gradle build complained about implementing a deprecated method

kantenkugel · 2017-10-04T14:30:44Z

src/main/java/net/dv8tion/jda/core/requests/ratelimit/BotRateLimiter.java

-                                  + "\nRoute: " + bucket.getRoute()
-                                  + "\nHeaders: " + headers);
+                Requester.LOG.debug("Encountered issue with headers when updating a bucket\nRoute: {}\nHeaders: {}",
+                    bucket.getRoute(), headers);


Ok if i remove the isDebugEnabled part?

Yes, doubt that check is needed.

… some logs into a single log statement

schnapster · 2017-10-18T10:53:13Z

src/main/java/net/dv8tion/jda/core/requests/WebSocketClient.java

@@ -938,7 +938,7 @@ protected void handleEvent(JSONObject raw)
 //        }

        JSONObject content = raw.getJSONObject("d");


Maybe move this into the try catch block below? I just got a JSONException on 286 from this line, missing a print of the faulty JSON.

[2017-10-18 12:30:31,889] [ DEBUG] [JDA RateLimit-Queue Pool - Thread 2] n.d.jda.core.requests.Requester: Received response with following cf-rays: [3afad2a07e74646f-FRA] [2017-10-18 12:30:47,770] [ ERROR] [JDA [1 / 2] MainWS-ReadThread] n.d.j.core.requests.WebSocketClient: Encountered an Exception org.json.JSONException: JSONObject["d"] is not a JSONObject. at org.json.JSONObject.getJSONObject(JSONObject.java:641) at net.dv8tion.jda.core.requests.WebSocketClient.handleEvent(WebSocketClient.java:947) at net.dv8tion.jda.core.requests.WebSocketClient.onTextMessage(WebSocketClient.java:666) at com.neovisionaries.ws.client.ListenerManager.callOnTextMessage(ListenerManager.java:352) at com.neovisionaries.ws.client.ReadingThread.callOnTextMessage(ReadingThread.java:260) at com.neovisionaries.ws.client.ReadingThread.callOnTextMessage(ReadingThread.java:238) at com.neovisionaries.ws.client.ReadingThread.handleTextFrame(ReadingThread.java:963) at com.neovisionaries.ws.client.ReadingThread.handleFrame(ReadingThread.java:746) at com.neovisionaries.ws.client.ReadingThread.main(ReadingThread.java:108) at com.neovisionaries.ws.client.ReadingThread.runMain(ReadingThread.java:64) at com.neovisionaries.ws.client.WebSocketThread.run(WebSocketThread.java:45) [2017-10-18 12:31:21,579] [ DEBUG] [fredboat.FredBoat.main()] n.d.jda.core.requests.Requester: Received response with following cf-rays: [3afad3d719096361-FRA]

Not sure if this is the right PR to do that in

Seing as this is already fixed in another pr, not gonna implement here

That cannot be moved into the try/catch block because the json is used within the catch

Aight. My intent on suggesting that was less concerned with the current bug, more of a general idea to improve future logging.
And Minn is correct, the change is more complicated than moving the two lines into the try catch, I'm sure whoever decides to add this in here or in another PR will figure out something appropriate. Maybe look at other similar places for possible uncaught JSONExceptions and have PR for that after this is merged.

MinnDevelopment

lgtm

Almighty-Alpaca

Other than those two minor thing this pr looks good to me, too.

Almighty-Alpaca · 2017-11-12T18:45:09Z

src/main/java/net/dv8tion/jda/core/utils/package-info.java

@@ -18,7 +18,7 @@
 * Package which contains all utilities for the JDA library.
 * These are used by JDA itself and can also be useful for the library consumer!
 *
- * <p>This contains the JDA logger implementation {@link net.dv8tion.jda.core.utils.SimpleLog SimpleLog}
+ * <p>This contains the JDA-specific Logger factory {@link net.dv8tion.jda.core.utils.JDALogger JDALogger}
 * <br>Which is currently planned to be rewritten to be more end-user friendly.


I guess the rewrite isn't planned but finished.

Almighty-Alpaca · 2017-11-12T18:50:03Z

src/main/java/net/dv8tion/jda/core/utils/JDALogger.java

+        String getString() throws Exception;
+    }
+
+    private JDALogger() {


Shouldn't the inner class be at the end of the file?

…rror occurs outside of handling Applied review suggestions

kantenkugel added 6 commits September 28, 2017 19:29

Started with SLF4J stage 2. Added SLF4J Logger (fallback) and Factory

9ee270e

Moved SimpleLogger to jda pkg, made it package private, removed jdocs

78d0fa7

Added SLF4J to the README

eb9b830

First iteration of replacing string concats of loggers with lazy para…

71aba77

…meters

Added docs, fixed formatting, added lazy string construction

6feb979

Further improved docs in JDALogger and added priv constructor

30174cf

kantenkugel added status: completed has been completed but is not yet released requires testing labels Oct 1, 2017

kantenkugel added this to the Upcoming Release milestone Oct 1, 2017

MinnDevelopment self-requested a review October 1, 2017 14:24

MinnDevelopment modified the milestones: Upcoming Release, Release 3.4 Oct 3, 2017

MinnDevelopment reviewed Oct 4, 2017

View reviewed changes

kantenkugel commented Oct 4, 2017

View reviewed changes

Improved log messages for exceptions in AudioConnection and collapsed…

022187b

… some logs into a single log statement

schnapster reviewed Oct 18, 2017

View reviewed changes

MinnDevelopment added status: freezer this is currently put on hold and removed status: completed has been completed but is not yet released labels Nov 5, 2017

kantenkugel added 2 commits November 12, 2017 15:24

merged in current master and fixed conflicts/SimpleLog calls

2aa3663

Fixed some logger calls to properly use "{}"

7fb842c

MinnDevelopment removed the status: freezer this is currently put on hold label Nov 12, 2017

MinnDevelopment approved these changes Nov 12, 2017

View reviewed changes

Almighty-Alpaca approved these changes Nov 12, 2017

View reviewed changes

MinnDevelopment added 2 commits November 12, 2017 21:29

Added try/catch in onTextMessage to print full JSON package when an e…

fc92163

…rror occurs outside of handling Applied review suggestions

Added handling for CHANNEL_UPDATE for a not existing category

bbb0ba2

kantenkugel merged commit e9399b7 into master Nov 13, 2017

MinnDevelopment deleted the jdalogger branch November 13, 2017 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SLF4J Logger rewrite part2 #492

SLF4J Logger rewrite part2 #492

kantenkugel commented Oct 1, 2017 •

edited

kantenkugel commented Oct 1, 2017

schnapster commented Oct 4, 2017

Shredder121 commented Oct 4, 2017

kantenkugel commented Oct 4, 2017

schnapster commented Oct 4, 2017

schnapster commented Oct 4, 2017

Shredder121 commented Oct 4, 2017

MinnDevelopment Oct 4, 2017

kantenkugel Oct 4, 2017

kantenkugel Oct 4, 2017

MinnDevelopment Oct 4, 2017

MinnDevelopment Oct 4, 2017

kantenkugel Oct 4, 2017

kantenkugel Oct 4, 2017 •

edited

MinnDevelopment Oct 4, 2017

schnapster Oct 18, 2017

kantenkugel Oct 18, 2017

kantenkugel Oct 18, 2017

MinnDevelopment Oct 18, 2017

schnapster Oct 18, 2017

MinnDevelopment left a comment

Almighty-Alpaca left a comment

Almighty-Alpaca Nov 12, 2017 •

edited

Almighty-Alpaca Nov 12, 2017 •

edited

		@@ -938,7 +938,7 @@ protected void handleEvent(JSONObject raw)
		// }

		JSONObject content = raw.getJSONObject("d");

SLF4J Logger rewrite part2 #492

SLF4J Logger rewrite part2 #492

Conversation

kantenkugel commented Oct 1, 2017 • edited

kantenkugel commented Oct 1, 2017

schnapster commented Oct 4, 2017

Shredder121 commented Oct 4, 2017

kantenkugel commented Oct 4, 2017

schnapster commented Oct 4, 2017

schnapster commented Oct 4, 2017

Shredder121 commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantenkugel Oct 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MinnDevelopment left a comment

Choose a reason for hiding this comment

Almighty-Alpaca left a comment

Choose a reason for hiding this comment

Almighty-Alpaca Nov 12, 2017 • edited

Choose a reason for hiding this comment

Almighty-Alpaca Nov 12, 2017 • edited

Choose a reason for hiding this comment

kantenkugel commented Oct 1, 2017 •

edited

kantenkugel Oct 4, 2017 •

edited

Almighty-Alpaca Nov 12, 2017 •

edited

Almighty-Alpaca Nov 12, 2017 •

edited