Upgrading open-tracing and jaeger #5742
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 29 files reviewed, 1 unresolved discussion (waiting on @haverma)
a discussion (no related file):
Some things are failing, I am debugging that. Opened this to give an idea of the extent of changes
There was a problem hiding this comment.
Reviewed 1 of 29 files at r1, 3 of 6 files at r2.
Reviewable status: 4 of 29 files reviewed, 5 unresolved discussions (waiting on @haverma)
projects/pom.xml, line 774 at r2 (raw file):
Quoted 17 lines of code…
<groupId>io.jaegertracing</groupId> <artifactId>jaeger-core</artifactId> <version>${jaeger.version}</version> <exclusions> <exclusion> <groupId>commons-logging</groupId> <artifactId>commons-logging</artifactId> </exclusion> </exclusions> </dependency> <dependency> <groupId>io.jaegertracing</groupId> <artifactId>jaeger-thrift</artifactId> <version>${jaeger.version}</version> </dependency>
Please keep these sorted alphabetically (move to io section).
projects/allinone/pom.xml, line 136 at r2 (raw file):
<groupId>io.jaegertracing</groupId> <artifactId>jaeger-core</artifactId> </dependency>
(move to io section throughout)
projects/allinone/src/main/java/org/batfish/allinone/AllInOne.java, line 152 at r2 (raw file):
.fromEnv().
why use the from environment variants if you are going to override the settings?
projects/batfish/src/main/java/org/batfish/bddreachability/BDDLoopDetectionAnalysis.java, line 62 at r2 (raw file):
Span span = GlobalTracer.get().buildSpan("BDDLoopDetectionAnalysis.detectLoops").start(); try (Scope scope = GlobalTracer.get().scopeManager().activate(span)) {
I don't quite understand this pattern. It sure looks the docs give a simpler way to do this:
Tracer.SpanBuilder.startActiveSpan(boolean finishOnClose) will create a new Span and will automatically set is as the active one for the current context.
import io.opentracing.Scope;
// Strongly encouraged to use them under try statements,
// to prevent ending up with the incorrect active Span
// in case of error.
try (Scope scope = tracer.buildSpan("foo").startActive(true)) {
scope.span().setTag(...);
scope.span().log(...);
}
// The 'foo' Span is finished at this point.
from https://opentracing.io/guides/java/scopes/#scope-objects
Codecov Report
@@ Coverage Diff @@
## master #5742 +/- ##
============================================
+ Coverage 71.40% 71.42% +0.01%
+ Complexity 33677 33671 -6
============================================
Files 2788 2788
Lines 139590 139704 +114
Branches 16790 16794 +4
============================================
+ Hits 99677 99786 +109
+ Misses 32050 32042 -8
- Partials 7863 7876 +13 |
There was a problem hiding this comment.
Reviewable status: 0 of 35 files reviewed, 4 unresolved discussions (waiting on @dhalperi and @haverma)
a discussion (no related file):
Previously, haverma (Harsh Verma) wrote…
Some things are failing, I am debugging that. Opened this to give an idea of the extent of changes
Nothing is failing now but opentracing-jaxrs2 (https://github.com/opentracing-contrib/java-jaxrs) is not logging the name of REST APIs in span db (instead I see serialize/deserialize in span names). Couldn't figure out the reason but may be it is not compatible with latest open-tracing APIs or it is not being initialized/setup correctly
Here deserialize/serialize should ideally be the REST API's name, I can see the correct name in IDE but not in Jaeger UI.
projects/pom.xml, line 774 at r2 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
<groupId>io.jaegertracing</groupId> <artifactId>jaeger-core</artifactId> <version>${jaeger.version}</version> <exclusions> <exclusion> <groupId>commons-logging</groupId> <artifactId>commons-logging</artifactId> </exclusion> </exclusions> </dependency> <dependency> <groupId>io.jaegertracing</groupId> <artifactId>jaeger-thrift</artifactId> <version>${jaeger.version}</version> </dependency>Please keep these sorted alphabetically (move to
iosection).
done
projects/allinone/pom.xml, line 136 at r2 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
<groupId>io.jaegertracing</groupId> <artifactId>jaeger-core</artifactId> </dependency>(move to
iosection throughout)
moved (alphabetized by groupID)
projects/allinone/src/main/java/org/batfish/allinone/AllInOne.java, line 152 at r2 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
.fromEnv().why use the
from environmentvariants if you are going to override the settings?
creating a new instance now
projects/batfish/src/main/java/org/batfish/bddreachability/BDDLoopDetectionAnalysis.java, line 62 at r2 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
Span span = GlobalTracer.get().buildSpan("BDDLoopDetectionAnalysis.detectLoops").start(); try (Scope scope = GlobalTracer.get().scopeManager().activate(span)) {I don't quite understand this pattern. It sure looks the docs give a simpler way to do this:
Tracer.SpanBuilder.startActiveSpan(boolean finishOnClose) will create a new Span and will automatically set is as the active one for the current context.
import io.opentracing.Scope; // Strongly encouraged to use them under try statements, // to prevent ending up with the incorrect active Span // in case of error. try (Scope scope = tracer.buildSpan("foo").startActive(true)) { scope.span().setTag(...); scope.span().log(...); } // The 'foo' Span is finished at this point.from https://opentracing.io/guides/java/scopes/#scope-objects
It seems that the linked doc is for JaegerTracer and not open-tracing's Tracer. Also in JaegerTracer, they have marked that API @deprecated - https://github.com/jaegertracing/jaeger-client-java/blob/a0080e065d4dbe805a9e02ed669759f042808023/jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java#L486
open-tracing's Tracer (which we are using) - https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java doesn't have startActive. So activating a span may be a two step process (with no auto closable)
There was a problem hiding this comment.
Reviewed 12 of 16 files at r3.
Reviewable status: 12 of 35 files reviewed, 3 unresolved discussions (waiting on @dhalperi and @haverma)
maven_install.json, line 4134 at r3 (raw file):
}, { "coord": "org.xerial:sqlite-jdbc:jar:sources:3.25.2",
Did you sort this file or something? This was not created simply by running @unpinned_maven//:pin, I think. At least my runs ~1 day ago did not rewrite the entire file.
There was a problem hiding this comment.
Reviewable status: 12 of 35 files reviewed, 3 unresolved discussions (waiting on @haverma)
projects/batfish/src/main/java/org/batfish/bddreachability/BDDLoopDetectionAnalysis.java, line 62 at r2 (raw file):
Previously, haverma (Harsh Verma) wrote…
It seems that the linked doc is for
JaegerTracerand not open-tracing'sTracer. Also in JaegerTracer, they have marked that API@deprecated- https://github.com/jaegertracing/jaeger-client-java/blob/a0080e065d4dbe805a9e02ed669759f042808023/jaeger-core/src/main/java/io/jaegertracing/internal/JaegerTracer.java#L486open-tracing's Tracer (which we are using) - https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java doesn't have
startActive. So activating a span may be a two step process (with no auto closable)
yep. wow, they have terrible documentation maintenance habits.
There was a problem hiding this comment.
Reviewed 19 of 29 files at r1, 4 of 16 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @haverma)
a discussion (no related file):
Previously, haverma (Harsh Verma) wrote…
Nothing is failing now but
opentracing-jaxrs2(https://github.com/opentracing-contrib/java-jaxrs) is not logging the name of REST APIs in span db (instead I seeserialize/deserializein span names). Couldn't figure out the reason but may be it is not compatible with latest open-tracing APIs or it is not being initialized/setup correctly
Here
deserialize/serializeshould ideally be the REST API's name, I can see the correct name in IDE but not in Jaeger UI.
ok for now.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dhalperi and @haverma)
maven_install.json, line 4134 at r3 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
Did you sort this file or something? This was not created simply by running @unpinned_maven//:pin, I think. At least my runs ~1 day ago did not rewrite the entire file.
no, I also ran bazel run @unpinned_maven//:pin. I ran it again just now after reverting my changes, and it again produced the same file.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @haverma)
open-tracing upgraded from 0.30 to 0.33
open-tracing-jaxrs upgraded from 0.0.9 to 1.0.0
jaeger core upgraded from 0.21 to 1.2.0 (group ID also changed for this)
The way spans are created changed.
ActiveSpanno longer exists, it is the same as any other span, and another classScopeManagerdecides which span is active.Now every span has to be closed by calling
span.finish()in finally.More details of usage can be found here