lightstep / lightstep-tracer-java

The Lightstep distributed tracing library for JRE
https://lightstep.com
MIT License
44 stars 24 forks source link

bundle logging is not working #223

Open zeitlinger opened 4 years ago

zeitlinger commented 4 years ago

The bundle does not have an slf4j implementation, hence logging is completely turned off, regardless of ls.verbosity.

Since lightstep-tracer-jre-bundle is self sufficient, it must provide an implementation of slf4j.

carlosalberto commented 4 years ago

hey @zeitlinger thanks for the report.

So at this moment we use Java's own logging facility, similar to what the SpecialAgent is doing. However, I've wondered in the past about using slf4j as the rest of the actual LS Tracer uses it. I will take a stab in the next few days.

safris commented 4 years ago

@carlosalberto, SpecialAgent actually uses its own home grown logging facility. This was done because reliance on Java Logging led to configuration conflicts with target applications, as well as a mixed bag of class loading issues. SpecialAgent must tread lightly with regard to its reliance on 3rd-party libs, because those libs may be present in the target application.

As for the LightStep Tracer Plugin, it should not be a problem to import slf4j libs. This is because Tracer Plugins are loaded in isolated class loaders that should not interfere with the target application. I say "should not", because: In the case of Spring or Spring Boot, the Spring runtime scans all loaders for beans and components. If the 3rd-party lib in question is in some way related to Spring, then Spring will try to load it. I do not believe that slf4j would cause such a situation.

carlosalberto commented 4 years ago

@safris Fantastic, thanks for chiming in. So I will update the build to include/load the slf4j dependency.

zeitlinger commented 4 years ago

I've tried this locally as well by adding this to the bundle:

      <dependency>
        <groupId>org.slf4j</groupId>
        <artifactId>slf4j-jdk14</artifactId>
        <version>1.7.30</version>
      </dependency>

This logs everything INFO and higher, which is the default for java logging.

Setting the verbosity to 4 (debug) has no effect, however. Makes me wonder why we have the duality (verbosity on the one hand and logging levels on the other) - very confusing without any apparent benefit as far as I can see.

carlosalberto commented 4 years ago

hey @zeitlinger

I will double check the entire code base. This is "legacy code" to some degree so I'm not sure about the actual reasons, but will be doing a full pass on the Java codebase.

safris commented 4 years ago

Hi @carlosalberto, is there an update re this issue?

carlosalberto commented 4 years ago

Hey @zeitlinger I've prepared a small PR that makes the bundle part that loads the LS Tracer use sl4fj (which is what actually uses the main LS Tracer anyway), and also includes the related slf4j dependency in the final bundle jar.

Setting the verbosity to 4 (debug) has no effect, however. Makes me wonder why we have the duality (verbosity on the one hand and logging levels on the other)

Could you clarify this part? sl4fj-simple is the only artifact with the INFO-and-upper limitation, so I guess you are referring about something else (including the verbosity vs logging levels).

FYI, I'm keeping the logging logic in the TracerFactory separated from the main Tracer one - the reason behind this is that creating a Tracer happens once and we want to be be explicit about any errors at all times.

Let me know what you think and I will fix this up in the next 1-2 days.

zeitlinger commented 4 years ago

I've prepared a small PR

great!

Setting the verbosity to 4 (debug) has no effect, however. Makes me wonder why we have the duality (verbosity on the one hand and logging levels on the other)

Could you clarify this part? sl4fj-simple is the only artifact with the INFO-and-upper limitation, so I guess you are referring about something else (including the verbosity vs logging levels).

my comment is not related to sl4fj-simple specifically.

These are the 2 places where a log statement can be suppressed

  1. when ls.verbosity is applied, e.g. here
  2. when slf4j is called later here

i.e. we could either

FYI, I'm keeping the logging logic in the TracerFactory separated from the main Tracer one - the reason behind this is that creating a Tracer happens once and we want to be be explicit about any errors at all times.

yes, that makes sense as far as I understand

arpit728 commented 4 years ago

@zeitlinger I also checked out the tracer codebase and I also had similar thoughts on ls.verbosity. I was also trying to understand rationale behind this, but couldn't got any idea.

It would make sense, to get rid of ls.verbosity.

Also I request you to checkout https://github.com/lightstep/lightstep-tracer-java/issues/229.

safris commented 4 years ago

Hi @carlosalberto, is there an update to this issue?

safris commented 4 years ago

Hi @zeitlinger, is this issue still relevant?

zeitlinger commented 4 years ago

Hi @zeitlinger, is this issue still relevant?

yes

safris commented 4 years ago

Hi @carlosalberto, is there an update to this issue?