opentracing-contrib / java-jdbc

OpenTracing Instrumentation for JDBC
Apache License 2.0
82 stars 56 forks source link

JdbcTracingUtils.decorate can set empty tag values which can break tracers #44

Closed randallt closed 5 years ago

randallt commented 5 years ago

I am using the Java SpecialAgent 1.3.2 with a Wavefront Tracer. The Wavefront Tracer does not support empty tag values (an empty String) and throws an exception when processing the span. The OpenTracing spec seems to imply that empty tag values should not be used, but doesn't specifically say they are disallowed:

From https://opentracing.io/specification/ :

Set a Span tag
Required parameters

The tag key, which must be a string
The tag value, which must be either a string, a boolean value, or a numeric type
Note that the OpenTracing project documents certain “standard tags” that have prescribed semantic meanings.

The code checks for null, but allows the empty String. This should be fixed:

private static void decorate(Span span,
      String sql,
      ConnectionInfo connectionInfo) {
    Tags.COMPONENT.set(span, COMPONENT_NAME);
    Tags.DB_STATEMENT.set(span, sql);
    if (connectionInfo.getDbType() != null) {
      Tags.DB_TYPE.set(span, connectionInfo.getDbType());
    }
    if (connectionInfo.getDbPeer() != null) {
      PEER_ADDRESS.set(span, connectionInfo.getDbPeer());
    }
    if (connectionInfo.getDbInstance() != null) {
      Tags.DB_INSTANCE.set(span, connectionInfo.getDbInstance());
    }
    if (connectionInfo.getDbUser() != null) {
      Tags.DB_USER.set(span, connectionInfo.getDbUser());
    }
  }
randallt commented 5 years ago

It looks like it is the 'db.statement' tag that is empty in my case. Somehow the sql parameter is empty or null here. This code should handle that.

randallt commented 5 years ago

Possible solution here:

private static boolean isNotEmpty(CharSequence s) {
      return s != null && !"".contentEquals(s);
  }

  private static void decorate(Span span,
      String sql,
      ConnectionInfo connectionInfo) {

      Tags.COMPONENT.set(span, COMPONENT_NAME);
      if (isNotEmpty(sql)) {
          Tags.DB_STATEMENT.set(span, sql);
      }
      if (isNotEmpty(connectionInfo.getDbType())) {
          Tags.DB_TYPE.set(span, connectionInfo.getDbType());
      }
      if (isNotEmpty(connectionInfo.getDbPeer())) {
          PEER_ADDRESS.set(span, connectionInfo.getDbPeer());
      }
      if (isNotEmpty(connectionInfo.getDbInstance())) {
          Tags.DB_INSTANCE.set(span, connectionInfo.getDbInstance());
      }
      if (isNotEmpty(connectionInfo.getDbUser())) {
          Tags.DB_USER.set(span, connectionInfo.getDbUser());
      }
  }
malafeev commented 5 years ago

@randallt would you like to submit a PR?

malafeev commented 5 years ago

fixed in #46

randallt commented 5 years ago

Looks good. Thanks.