Adding getter of operationName to Span

[quaff]

In my case, I need begin span in one method conditionally, and close it in another method, before close it I need check current active span is the previous one, using ThreadLocal to store span is not a graceful way, I prefer to check span name (make sure unique by application).

private static final String SPAN_NAME = "SOME_SPAN_NAME";

public void preAction() {
    boolean shouldBegin = true; // conditional
    if (shouldBegin) {
        Tracer tracer = GlobalTracer.get();
        if (tracer.activeSpan() != null) {
            Span span = tracer.buildSpan(SPAN_NAME).start();
            tracer.scopeManager().activate(span, true);
        }
    }
}

public void postAction() {
    Scope scope = GlobalTracer.get().scopeManager().active();
    if (scope != null) {
        Span span = scope.span();
        if (SPAN_NAME.equals(span.operationName())) {
            scope.close();
        }
    }
}

My workaround using BaggageItem:

private static final String SPAN_NAME = "SOME_SPAN_NAME";
private static final String BAGGAGE_NAME = "SOME_BAGGAGE_NAME";

public void preAction() {
    boolean shouldBegin = true; // conditional
    if (shouldBegin) {
        Tracer tracer = GlobalTracer.get();
        if (tracer.activeSpan() != null) {
            Span span = tracer.buildSpan(SPAN_NAME).start();
            span.setBaggageItem(BAGGAGE_NAME, "true");
            tracer.scopeManager().activate(span, true);
        }
    }
}

public void postAction() {
    Scope scope = GlobalTracer.get().scopeManager().active();
    if (scope != null) {
        Span span = scope.span();
        if ("true".equals(span.getBaggageItem(BAGGAGE_NAME))) {
            span.setBaggageItem(BAGGAGE_NAME, null); // Should we have an method removeBaggageItem ?
            scope.close();
        }
    }
}

[yurishkuro]

Doesn't your "postAction" have some sort of context telling it what it is it's support to finalize/cleanup? Having such a context is a common pattern when middleware is implemented as a pair of asymmetric interceptors, and you are typically allowed to store extra data in the context, like the span that was started in preAction.

For example, RequestInterceptor and ResponseInterceptor in Apache HTTP share the HttpContext local to the request - https://github.com/jaegertracing/legacy-client-java/blob/master/jaeger-apachehttpclient/src/main/java/com/uber/jaeger/httpclient/TracingRequestInterceptor.java#L47

[quaff]

My actual user case is tracing mysql execution by implementing com.mysql.jdbc.StatementInterceptorV2

import java.net.URI;
import java.sql.SQLException;
import java.util.HashMap;
import java.util.Map;
import java.util.Properties;

import com.mysql.jdbc.Connection;
import com.mysql.jdbc.PreparedStatement;
import com.mysql.jdbc.ResultSetInternalMethods;
import com.mysql.jdbc.Statement;
import com.mysql.jdbc.StatementInterceptorV2;

import io.opentracing.Scope;
import io.opentracing.Span;
import io.opentracing.Tracer;
import io.opentracing.log.Fields;
import io.opentracing.tag.Tags;
import io.opentracing.util.GlobalTracer;

public class MySQLStatementInterceptor implements StatementInterceptorV2 {

    private final static String BAGGAGE_NAME_MYSQL = "MYSQL";

    @Override
    public ResultSetInternalMethods preProcess(String sql, Statement interceptedStatement, Connection connection) {
        if (interceptedStatement instanceof PreparedStatement)
            sql = ((PreparedStatement) interceptedStatement).getPreparedSql();
        int spaceIndex = sql.indexOf(' ');
        Tracer tracer = GlobalTracer.get();
        if (tracer.activeSpan() != null) {
            // only start new span in parent span
            Span span = tracer
                    .buildSpan("mysql: " + (spaceIndex == -1 ? sql : sql.substring(0, spaceIndex)).toLowerCase())
                    .start();
            Tags.SPAN_KIND.set(span, Tags.SPAN_KIND_CLIENT);
            span.setTag("db.query", sql);
            span.setBaggageItem(BAGGAGE_NAME_MYSQL, "true");
            parseServerAddress(connection, span);
            tracer.scopeManager().activate(span, true);
        }
        return null;
    }

    @Override
    public ResultSetInternalMethods postProcess(String sql, Statement interceptedStatement,
            ResultSetInternalMethods originalResultSet, Connection connection, int warningCount, boolean noIndexUsed,
            boolean noGoodIndexUsed, SQLException statementException) {
        Scope scope = GlobalTracer.get().scopeManager().active();
        if (scope != null) {
            Span span = scope.span();
            if ("true".equals(span.getBaggageItem(BAGGAGE_NAME_MYSQL))) {
                span.setBaggageItem(BAGGAGE_NAME_MYSQL, null);
                if (statementException != null) {
                    Tags.SAMPLING_PRIORITY.set(span, 1);
                    Tags.ERROR.set(span, true);
                    Map<String, Object> map = new HashMap<>();
                    map.put(Fields.EVENT, "error");
                    map.put(Fields.ERROR_OBJECT, statementException);
                    map.put(Fields.MESSAGE, statementException.getMessage());
                    span.log(map);
                }
                scope.close();
            }
        }
        return null;
    }

    static void parseServerAddress(Connection connection, Span span) {
        try {
            URI uri = URI.create(connection.getMetaData().getURL().substring(5));
            Tags.PEER_HOSTNAME.set(span, uri.getHost() == null ? "localhost" : uri.getHost());
            Tags.PEER_PORT.set(span, uri.getPort() == -1 ? 3306 : uri.getPort());
            Tags.DB_INSTANCE.set(span, uri.getPath().substring(1));
        } catch (Exception e) {

        }
    }

    @Override
    public boolean executeTopLevelOnly() {
        return true;
    }

    @Override
    public void init(Connection conn, Properties props) {

    }

    @Override
    public void destroy() {

    }
}

[jpkrohling]

Sounds absolutely reasonable to me, especially given that there's a setter for the operation name :)

[yurishkuro]

I am not so sure that the presence of setters is a justification to add getters. OT API has no getters today, except for baggage that genuinely needs it. This design, for example, allows implementations that flush all span data as micro-events (cf. jaeger #729 - support incomplete spans).

In this specific example, relying on the span name also sounds like a hack due to insufficient framework support in mysql. Looking at the API, it seems returning an implementation of ResultSetInternalMethods is the only way to return some object that would be given back to the postProcess(), but that API is huge.

My advise would be to use a thread local, I don't see what's inelegant about that (the tracer's scope manager also uses thread local).

A separate question - why do you need to instrument mysql connector directly instead of say JDBC (like https://github.com/opentracing-contrib/java-jdbc)?

[quaff]

@yurishkuro I didn't notice java-jdbc before, I'm going to use it instead of mysql StatementInterceptor.

[jpkrohling]

This design, for example, allows implementations that flush all span data as micro-events

Just out of curiosity: is this by accident, or was it a conscious decision to not provide getters at the API level?

[yurishkuro]

It was absolutely a conscious decision, Span API is write-only. Allowing getters on the span puts additional constraints on the implementors, and we have not yet seen a valid use case where getters would be necessary.

[jpkrohling]

Span API is write-only

It makes sense now, but I don't remember seeing this before. Perhaps this issue could be converted into a doc issue?

[felixbarny]

Related: https://github.com/opentracing/opentracing-java/issues/57