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

Feature proposal : Integrate a more advanced logging framework #1340

Closed
sheinbergon opened this issue Jul 6, 2016 · 13 comments
Closed

Feature proposal : Integrate a more advanced logging framework #1340

sheinbergon opened this issue Jul 6, 2016 · 13 comments

Comments

@sheinbergon
Copy link
Contributor

sheinbergon commented Jul 6, 2016

Though I understand the considerations of keeping Jedis lean & mean and the overall aim of not binding to specific logging frameworks in 3rd party libraries in the likes of Jedis, I find the JUL interface to be rather crude / obsolete .

Plus , it pales in comparison to other logging solutions performance wise :

http://blog.takipi.com/the-logging-olympics-a-race-between-todays-top-5-logging-frameworks/
https://logging.apache.org/log4j/2.x/performance.html

I have very good experience with Log4J2 in other projects I use it.

@marcosnils @HeartSaVioR was this ever discussed ?

@xetorthio
Copy link
Contributor

Never discussed before. I wonder what other big java projects are using...

@marcosnils
Copy link
Contributor

It has been discussed indeed. #1219

@HeartSaVioR
Copy link
Contributor

Yeah discussed before. Note that we're only using log to JedisSentinelPool.

@sheinbergon
Copy link
Contributor Author

sheinbergon commented Jul 8, 2016

@HeartSaVioR @marcosnils I've gone over #1219 thoroughly , the arguments there indeed make sense ( and not surprising at all ) .

If I were to implement an internal logger factory as @mp911de mentioned (and portrayed here https://github.com/netty/netty/blob/4.1/common/src/main/java/io/netty/util/internal/logging/InternalLoggerFactory.java#L51 ) but make it a bit smartter ( like support placeholders in a unified fashion ) -

Is that something you'd be inclined to merge into jedis ? it won't effect the dependency graph ( will only work if dependencies will be marked as provided ) and would allow for a nicer/richer logging interface ?

Hoping for a yes :) let me know :)

@marcosnils
Copy link
Contributor

@sheinbergon @HeartSaVioR I do agree that as Jedis / Redis is becoming more complex (with cluster and modules) it'd be nice to add logging in order to print some debug information when needed. @sheinbergon if dependency graph is kept small and adding logging doesn't require too much effort, I believe that we can include it and use it when needed.

@sheinbergon
Copy link
Contributor Author

@marcosnils that's great . Let's leave this issue open , I'll work on it as soon as I clear some other stuff from the table

@r0main
Copy link
Contributor

r0main commented Feb 10, 2017

Hi,

Which logging interface do you want to use ?

I think slf4j would be the best suited for this, slf4j-api has no transitive dependencies (https://search.maven.org/#artifactdetails|org.slf4j|slf4j-api|1.7.22|jar). Every project could then use the implementation he wants (logback, log4j2, ...).

I see there is only 3 classes using JUL it's not much work to migrate to slf4j, I would be happy to propose a pull request. Just tell me which logging interface you want to use.

r0main added a commit to r0main/jedis that referenced this issue Feb 17, 2017
Use log4j2 with slf4j bridge for testing
@mballoni
Copy link

I agree with @r0main .
Also even if there was a lot of usage of some log library you could use the slf4j migration tool:
https://www.slf4j.org/migrator.html

@r0main
Copy link
Contributor

r0main commented Feb 20, 2017

@mballoni I didn't knew that migration tool, thanks. Anyway there are a lot of limitations with JUL that need a manual resolution. I made a pull request few days ago without the tool, feel free to review : #1467

@mballoni
Copy link

@r0main I saw your PR. It looks good, avoiding string concatenations and toString invocations.
Just to see if everybody agrees with the chosen log levels.

@r0main
Copy link
Contributor

r0main commented Feb 20, 2017

@mballoni I applied this concersion table : https://www.slf4j.org/apidocs/org/slf4j/bridge/SLF4JBridgeHandler.html So people already bridging JUL with slf4j shouldn't see a difference.

@sheinbergon
Copy link
Contributor Author

@mballoni #1340 (comment) my point exactly

HeartSaVioR pushed a commit that referenced this issue Feb 22, 2017
Use log4j2 with slf4j bridge for testing
HeartSaVioR pushed a commit that referenced this issue Feb 22, 2017
Use log4j2 with slf4j bridge for testing
@sheinbergon
Copy link
Contributor Author

PR is marked as merged. closing

werkt pushed a commit to werkt/jedis that referenced this issue May 15, 2020
werkt pushed a commit to werkt/jedis that referenced this issue Jul 23, 2020
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

No branches or pull requests

6 participants