Make GlobalTracer optional

[malafeev]

It looks not possible because of https://github.com/opentracing-contrib/java-jdbc/blob/master/src/main/java/io/opentracing/contrib/jdbc/TracingDriver.java#L37:

static {
    try {
      DriverManager.registerDriver(INSTANCE);
    } catch (SQLException e) {
      throw new IllegalStateException("Could not register TracingDriver with DriverManager", e);
    }
}

[KoenDG]

Just started using this library. Making GlobalTracer optional would be really helpful.

Right now, it just logs the queries with no context at all as to how they were generated.

Imagine working with a REST api where each call to the API is handled in its own thread and therefor, each thread gets its own tracer. It would be super useful to tell the library to use that tracer for that thread.

With GlobalTracer, you lose context as to which thread generated the query. With GlobalTracer, each query is just logged on its own, with nothing linking it to the thread that generated it.

If not possible, how about adding the option for adding a stacktrace to the span? At least what way, we can see the code path used to arrive at the query.

[malafeev]

each thread gets its own tracer

Ideally tracer should be only one per application (jvm instance). GlobalTracer solves the problem of passing the same tracer to all parts of code.

Your REST endpoints should be instrumented also. Then jdbc span will use REST span as parent span. So you will see chain of spans (REST->jdbc). It will give you idea of trace path.

[KoenDG]

Ideally tracer should be only one per application (jvm instance). GlobalTracer solves the problem of passing the same tracer to all parts of code.

I'll look into it. So far, my attempts at using a static tracer have resulted in there being 1 single trace in jaeger and it containing more and more spans as more calls come in.

Which isn't really clear. Having each call in its own trace would be more clear, with the starting time and all that.

I'll try and set up a demo project based on what I'm doing and present that. I'm clearly doing something wrong somewhere.

[KoenDG]

@malafeev I made a demo project here: https://github.com/KoenDG/play-java-ebean-example/tree/tracingAttempt

On a branch called tracingAttempt.

Diff with original can be seen here: https://github.com/KoenDG/play-java-ebean-example/compare/2.6.x...KoenDG:tracingAttempt

So I run my code, I run the jaeger docker image like this: docker run -p5775:5775/udp -p6831:6831/udp -p6832:6832/udp -p16686:16686 -p14268:14268 jaegertracing/all-in-one:latest

No daemon so I can do a clean launch quick while testing the code.

I got to localhost:9000 and refresh a bunch of times to create traces.

And the result I'm getting is this: https://imgur.com/a/iQ4eA2C

So, each call is getting its own trace, which is what I would expect: a contextually separated, clickable instance in the jaeger overview, per call. Which basically tells me "this call did the following".

I tried putting everything on globaltracer and then you run into the problem of calls not being separated. Everything just ends up in 1 big trace, which isn't usable in a multithreaded environment, processing potentially the same call several times, but from different callers providing different details.

While the jdbc traces are there, you can't tell what they're a child of. As the various database calls are all on the same indentation/level of the view.

So, if I want to achieve a single trace for each call the HTTP server gets, with the database queries in there, what am I doing wrong?

I'll be asking this on the play framework's forum as well, in case something specific is needed.

EDIT: Topic on Play Framework's forum: https://discuss.lightbend.com/t/tracing-part-2/1026

[malafeev]

@KoenDG I cannot add comments to diff you provided. Could you create PR from tracingAttempt branch to 2.6.x branch, so I will be able to comment?

[KoenDG]

@malafeev My bad, here's the PR: https://github.com/KoenDG/play-java-ebean-example/pull/1

[yurishkuro]

This library currently only seems to support instantiation via driver class name, which is typically not recommended for any serious applications using any sort of framework. The recommended way is via DataSource, which allow the framework to manage the life cycle of the objects. The best solution is to pull out the instrumentation portion out of the TracingDriver class into another class that accepts the tracer as an argument.

[malafeev]

That's not true. Usually you never instantiate the driver. It should be only in the classpath. This library works with Spring, Hibernate, plain jdbc. Please look at tests.

[yurishkuro]

BasicDataSource dataSource = new BasicDataSource(); dataSource.setDriverClassName("io.opentracing.contrib.jdbc.TracingDriver");

This is from Spring example - still creates driver by class name.

[malafeev]

you can remove this line, and tests will pass.

[malafeev]

but I still don't understand complain, DataSource configuration accepts driver class name. If you think it's a bad practice it's not issue of this lib.

[yurishkuro]

The issue is dependency on global tracer. The only situation where it's necessary is when people load driver by class name. This lib can provide an implementation of DataSource that can be instantiated as a normal class (no statics or singletons of any sort) and accept the tracer as an argument. Then the existing driver mechanism can use that to initialize its singleton with global tracer, to not affect existing users.

[malafeev]

In some cases you don't instantiate DataSource, you just declare it via configuration file (e.g. Spring Boot). If you can instantiate datasource, you also can instantiate TracingDriver and provide tracer via setter (which exists) to avoid GlobalTracer.

[yurishkuro]

Here's an example where tracer is not passed and the JdbcUtils class will fall back onto global tracer:

https://github.com/opentracing-contrib/java-jdbc/blob/08c40eff7a8cae98069339bbd24226e8b6200a3e/src/main/java/io/opentracing/contrib/jdbc/TracingConnection.java#L73-L76

While it's easy to fix, issues like that do not inspire confidence that this lib is ready for prod use. What I am proposing is minor re-design that makes it impossible for these bugs to exist, because the only place in the code that will ever fall back to global tracer is the Driver singleton

• everything accepts tracer as @Notnull argument
• remove getNullsafeTracer() completely - there's only one place where null check needs to be made, in the Driver

[malafeev]

• @NotNull is not supported by java 7 (We have to upgrade to java 8 at least)
• remove getNullsafeTracer() completely - sounds good

[asvanberg]

This issue can be closed since the dependency on GlobalTracer is long gone and there's a tracing implementation of java.sql.DataSource