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.

Sign up for GitHub

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

Jump to bottom

Redirect netty logging to driver logging #437

Merged
merged 3 commits into from
Nov 29, 2017
Merged

Redirect netty logging to driver logging #437

merged 3 commits into from
Nov 29, 2017

Conversation

zhenlineo
Copy link
Contributor

@zhenlineo zhenlineo commented Nov 28, 2017 •
edited
Loading

An example of output on DEBUG (FINE) level using our ConsoleLogging:

2017-11-28 05:53:19,431 [0xb2d66fa6] S: SUCCESS {}
2017-11-28 05:53:19,431 Channel [id: 0xb2d66fa6, L:/127.0.0.1:55381 - R:localhost/127.0.0.1:7687] released back to the pool
2017-11-28 05:53:19,433 Channel [id: 0xb2d66fa6, L:/127.0.0.1:55381 - R:localhost/127.0.0.1:7687] acquired from the pool
2017-11-28 05:53:19,434 [0xb2d66fa6] C: RUN "MATCH (n) WITH n LIMIT 10000 DETACH DELETE n RETURN count(n)" {}
2017-11-28 05:53:19,434 [0xb2d66fa6] C: PULL_ALL
2017-11-28 05:53:19,438 [0xb2d66fa6] S: SUCCESS {fields=["count(n)"], result_available_after=1}
2017-11-28 05:53:19,438 [0xb2d66fa6] S: RECORD [0]
2017-11-28 05:53:19,438 [0xb2d66fa6] S: SUCCESS {result_consumed_after=0, type="rw"}
2017-11-28 05:53:19,438 [0xb2d66fa6] C: RESET
2017-11-28 05:53:19,440 [0xb2d66fa6] S: SUCCESS {}
2017-11-28 05:53:19,440 Channel [id: 0xb2d66fa6, L:/127.0.0.1:55381 - R:localhost/127.0.0.1:7687] released back to the pool
2017-11-28 05:53:19,468 [id: 0x796463d7, L:/127.0.0.1:55382 - R:localhost/127.0.0.1:7687] HANDSHAKEN: TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
2017-11-28 05:53:19,508 Closing driver instance org.neo4j.driver.internal.InternalDriver@3108bc
2017-11-28 05:53:19,509 Closing connection pool towards localhost:7687
2017-11-28 05:53:21,633 Freed 19 thread-local buffer(s) from thread: Neo4jDriverIO-4-2

@zhenlineo zhenlineo requested a review from lutovich November 28, 2017 16:53
@zhenlineo zhenlineo force-pushed the 1.5-redirect-logging branch from 62d3ce7 to 11c1a2f Compare November 29, 2017 09:10
Zhen added 2 commits November 29, 2017 13:35
Added a simple DelegateLogging to directly pass netty's log into our …
4049b28 
…logging system

While as netty use `{}` instead of `%s`, we cannot print their log message properly.
This PR did not address the conversion between two logging systems.
Direct netty logging to driver logging using a simple adaptor
3dcf9c5
@zhenlineo zhenlineo force-pushed the 1.5-redirect-logging branch from 11c1a2f to 3dcf9c5 Compare November 29, 2017 12:40
lutovich
lutovich reviewed Nov 29, 2017
View reviewed changes
Copy link
Contributor

@lutovich lutovich left a comment

Choose a reason for hiding this comment

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

@zhenlineo review completed. Made some comments & suggestions.

driver/src/main/java/org/neo4j/driver/internal/InternalDriver.java
@@ -112,7 +112,7 @@ public void close()
{
if ( closed.compareAndSet( false, true ) )
{
log.info( "Driver instance is closing" );
log.info( "Closing driver instance %s", this );
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth adding info log where driver is created? Otherwise printing of this does not seem very valuable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added another logging in constructor

driver/src/main/java/org/neo4j/driver/internal/logging/ConsoleLogging.java
// builder.append( "[" ).append( record.getSourceClassName() ).append( "." );
// builder.append( record.getSourceMethodName() ).append( "] - " );
// builder.append( "[" ).append( record.getLevel() ).append( "] - " );
// builder.append("[").append(record.getLoggerName()).append("] ");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this line be uncommented? Can we remove next 3 commented lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ConsoleLogging class is just for internal use purpose. I would prefer keep these commented there in case if we ever need to print more info to help debug.

driver/src/main/java/org/neo4j/driver/internal/logging/DelegateLogger.java Outdated
import static java.lang.String.format;


public class DelegateLogger implements InternalLogger
Copy link
Contributor

Choose a reason for hiding this comment

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

How about calling this smth like NettyLogger?
I think we could also extend io.netty.util.internal.logging.AbstractInternalLogger to reduce amount of code.

driver/src/main/java/org/neo4j/driver/internal/logging/DelegateLogger.java Outdated
@Override
public String name()
{
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This class could take name as another constructor argument and return it here.

driver/src/main/java/org/neo4j/driver/internal/logging/DelegateLogging.java Outdated
/**
* This is the logging factory to delegate netty's logging to our logging system
*/
public class DelegateLogging extends InternalLoggerFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

How about calling this smth like NettyLogging?

driver/src/main/java/org/neo4j/driver/internal/logging/DelegateLogger.java Outdated

private String toDriverLoggerFormat( String format )
{
return format.replaceAll( "\\{\\}", "%s" );
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better for performance to precompile pattern with Pattern.compile("\\{\\}") into a static final constant and then use it like PATTERN.matcher(format).replaceAll("%s").

@lutovich lutovich merged commit 9765378 into neo4j:1.5 Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lutovich lutovich lutovich left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zhenlineo @lutovich