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

JdbcAspect.getConnection will execute SELECT USER() every time #173

Closed
dozer47528 opened this issue Sep 7, 2018 · 9 comments · Fixed by #195
Closed

JdbcAspect.getConnection will execute SELECT USER() every time #173

dozer47528 opened this issue Sep 7, 2018 · 9 comments · Fixed by #195

Comments

@dozer47528
Copy link

dozer47528 commented Sep 7, 2018

I find the getConnection() method is slower than expected.
So I check the source code:

  public Object getConnection(final ProceedingJoinPoint pjp) throws Throwable {
    Connection conn = (Connection) pjp.proceed();
    String url = conn.getMetaData().getURL();
    String user = conn.getMetaData().getUserName();
    String dbType;
    boolean withActiveSpanOnly = false;
    try {
      dbType = url.split(":")[1];
    } catch (Throwable t) {
      throw new IllegalArgumentException(
          "Invalid JDBC URL. Expected to find the database type after the first ':'. URL: " + url);
    }
    return new TracingConnection(conn, dbType, user, withActiveSpanOnly);
  }

For this line: String user = conn.getMetaData().getUserName();
mysql-connector-java will execute SELECT USER() every time.

@dozer47528
Copy link
Author

Actually, I think it's hard for opentracing-spring-cloud-jdbc-starter to fix this issue.
opentracing-spring-cloud-jdbc-starter should follow the standard of JDBC, this behavior implemented by mysql-connector-java.

I resolved this issue by add this argument to mysql jdbc url:

jdbc:mysql://localhost:3306/test?useHostsInPrivileges=false

After changing useHostsInPrivileges to false, mysql-connector-java can get the username from memory directly.

    /**
     * What's our user name as known to the database?
     * 
     * @return our database user name
     * @throws SQLException
     */
    public String getUserName() throws SQLException {
        if (this.conn.getUseHostsInPrivileges()) {
            Statement stmt = null;
            ResultSet rs = null;

            try {
                stmt = this.conn.getMetadataSafeStatement();

                rs = stmt.executeQuery("SELECT USER()");
                rs.next();

                return rs.getString(1);
            } finally {
                if (rs != null) {
                    try {
                        rs.close();
                    } catch (Exception ex) {
                        AssertionFailedException.shouldNotHappen(ex);
                    }

                    rs = null;
                }

                if (stmt != null) {
                    try {
                        stmt.close();
                    } catch (Exception ex) {
                        AssertionFailedException.shouldNotHappen(ex);
                    }

                    stmt = null;
                }
            }
        }

        return this.conn.getUser();
    }

@wuyupengwoaini
Copy link
Contributor

I think the userName is not necessary at all,because normally application accesses the database by a fixed user. And then we can replace the userName by database ip and port because this is helpful to draw topology.

so the opentracing-jdbc project should support more info about database such as database ip and port

In design level, I think opentracing-jdbc is not flexiable beacuse of it's construction method.So i suggest we need to improve the opentracing-jdbc firstly and then add database ip and port to acted as tag k-v.

@wuyupengwoaini
Copy link
Contributor

@jpkrohling

@wuyupengwoaini
Copy link
Contributor

Now,TracingConnnection receieve a ConnectionInfo Object containing all the database information(instance,ip,port,type,username).So username is not required.And then we can reslove the database information by parse the jdbc url.For example the skywalking is doing so.

https://github.com/apache/incubator-skywalking/blob/master/apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/connectionurl/parser/ConnectionURLParser.java

@wuyupengwoaini
Copy link
Contributor

wuyupengwoaini commented Jan 14, 2019

@dozer47528 why close this issue,i think we need a pr to solve this problem

@wuyupengwoaini
Copy link
Contributor

@pavolloffay what do you think about my suggestion?

@dozer47528 dozer47528 reopened this Jan 14, 2019
@pavolloffay
Copy link
Contributor

@wuyupengwoaini would you like to contribute a fix with your solution?

@wuyupengwoaini
Copy link
Contributor

Ok, i'll try to fix it within a few days

@wuyupengwoaini
Copy link
Contributor

@pavolloffay please help me to review the pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants