Update lessons #30

safris · 2018-09-06T17:04:37Z

Hi @yurishkuro, I went through the lessons in this repository yesterday. The changes in this pull request identify the changes to the OpenTracing APIs since the lessons were written. I also updated run.sh to allow the lessons to run on jdk9+.

yurishkuro · 2018-09-06T21:29:21Z

java/src/main/java/lesson01/README.md

@@ -101,7 +101,7 @@ Our `pom.xml` already imports Jaeger:
 <dependency>
    <groupId>com.uber.jaeger</groupId>
    <artifactId>jaeger-core</artifactId>
-    <version>0.26.0</version>
+    <version>0.27.0</version>


we should change it to io.jaegertracing

Oh, ok. I didn't know this package has been moved to io.jaegertracing:jaeger-core. The java package names have changed, so imports and fully qualified class references would have to be updated too. I can update this pull request with these changes as well.

+1

I think you have noticed as well, but we are on 0.310. already, so, a version bump to that one would be good.

yurishkuro · 2018-09-06T21:29:39Z

java/src/main/java/lesson01/solution/Hello.java

-        Tracer tracer = Tracing.init("hello-world");
-        new Hello(tracer).sayHello(helloTo);
-        tracer.close();
+        try (Tracer tracer = Tracing.init("hello-world")) {


is the tracer closable?

Tracer is com.uber.jaeger.Tracer in this class, which implements the Closeable interface. io.opentracing.Tracer, however, does not extend the Closeable interface.

[off-topic] Would be a good improvement for OpenTracing :)

jpkrohling · 2018-09-07T08:24:33Z

java/src/main/java/lesson01/README.md

-    SamplerConfiguration samplerConfig = new SamplerConfiguration("const", 1);
-    ReporterConfiguration reporterConfig = new ReporterConfiguration(true, null, null, null, null);
-    Configuration config = new Configuration(service, samplerConfig, reporterConfig);
+    SamplerConfiguration samplerConfig = new SamplerConfiguration().withType("const").withParam(1);


How about we use a simple Configuration.fromEnv(serviceName).getTracer() here in the code and export the relevant env vars?

@jpkrohling, I just started learning OpenTracing, so I'm not familiar with "export the relevant env vars", yet.

Fair enough. This is particular to Jaeger (not generic OpenTracing), and it might indeed confuse users. For reference, I'm talking about JAEGER_REPORTER_LOG_SPANS, JAEGER_SAMPLER_TYPE and JAEGER_SAMPLER_PARAM

Could you change this to use fromEnv(), like what we are doing in the Katacoda scenario? We override some env vars there, so, fromEnv() is required there (even though it's not required here).

https://github.com/katacoda-scenarios/opentracing-scenarios/blob/master/opentracing-tutorial-lesson01/03-initialize-real-tracer.md

safris · 2018-09-07T10:10:54Z

Hi @yurishkuro and @jpkrohling, I pushed a commit that upgrades to io.jaegertracing:jaeger-core:0.31.0.

jpkrohling · 2018-09-07T10:25:20Z

java/src/main/java/lesson01/solution/Hello.java

-        Tracer tracer = Tracing.init("hello-world");
-        new Hello(tracer).sayHello(helloTo);
-        tracer.close();
+        try (JaegerTracer tracer = Tracing.init("hello-world")) {


IMO, the tutorial should be using OT's Tracer instead of JaegerTracer as much as possible. The side effect is that, at least for now, we'd need to close the tracer manually (see jaegertracing/jaeger-client-java#544).

That said, we could change the tutorial later, once we fix the mentioned issue.

I absolutely agree! I have changed the code and lesson01/README.md to reflect this.

java/src/main/java/lesson02/README.md

safris · 2018-09-07T18:21:19Z

Hi @yurishkuro and @jpkrohling, I caught a mistake in the work I had done to upgrade Jaeger to 0.31.0. The mistake was that the lessons stopped communicating with Jaeger that's running in Docker. The reason for this is due to the fact that in 0.31.0, it seems that the service provider for SenderFactory does not live in jaeger-core any more. It took a bit of digging, but I found ThriftSenderFactory in jaeger-thrift. This was elusive, because there were no compile-time or runtime errors to help me realize something was wrong. The reason I realized something was wrong is by noticing the "No suitable sender found. Using NoopSender, meaning that data will not be sent anywhere!" WARN message in the log. I have updated pom.xml and the README.md.

yurishkuro · 2018-09-07T18:36:04Z

@SevaSafris use jaeger-client instead of jaeger-core, the former pulls all needed dependencies

java/src/main/java/lesson03/exercise/Hello.java

yurishkuro · 2018-09-07T20:27:24Z

if you're removing the calls to tracer.close(), you will not get clean traces, unless we implement JVM shutdown hook in Jaeger.

safris · 2018-09-08T07:01:37Z

@yurishkuro, regarding tracer.close(): The close method is defined in io.jaegertracing.internal.JaegerTracer, but it is not defined in io.opentracing.Tracer. If we keep the tracer.close() method call, then the Hello class becomes implementation specific to Jaeger as the OT vendor. Perhaps the close() method should be added to io.opentracing.Tracer?

jpkrohling · 2018-09-10T08:05:22Z

@yurishkuro said:

if you're removing the calls to tracer.close(), you will not get clean traces, unless we implement JVM shutdown hook in Jaeger.

I submitted a PR last week to add this. It should be available in the next version of the client.

@SevaSafris said:

Perhaps the close() method should be added to io.opentracing.Tracer?

I believe so too, but this should probably be discussed/suggested as an issue opened against https://github.com/opentracing/opentracing-java . Would you be interested in starting a discussion there about this?

safris · 2018-09-10T09:41:26Z

@yurishkuro, I submitted a new issue for discussion here.

jpkrohling · 2018-09-27T10:22:24Z

What's the status of this? I'd like to run through the Katacoda scenarios and see if anything is broken, as it uses this code here.

jpkrohling · 2018-09-27T10:24:12Z

java/src/main/java/lesson02/solution/HelloActive.java

@@ -45,6 +45,5 @@ public static void main(String[] args) {
        String helloTo = args[0];
        Tracer tracer = Tracing.init("hello-world");
        new HelloActive(tracer).sayHello(helloTo);
-        tracer.close();


You'll still need this until we release 0.31.1, which includes the JVM shutdown hook.

I have been waiting for opentracing/opentracing-java#250 and opentracing/specification#128 to move forward with the Closeable spec, but it seems this will take a while. I've updated this pull request to upcast to JaegerTracer in a try-resource block.

jpkrohling · 2018-09-28T09:32:03Z

This LGTM, but please let's not get it merged until at least Tuesday, as @yurishkuro is presenting on Monday and will use this repo. As the Katacoda scenarios use the same code base, this has potential to break the scenarios.

I'll try to get the scenarios up to date next week, in line with this PR, unless @SevaSafris wants to give it a shot as well. The repository is this: https://github.com/katacoda-scenarios/opentracing-scenarios

safris · 2018-10-04T09:54:21Z

@jpkrohling, apologies for the delayed response -- I'm just getting over a cold. Regarding the scenarios: I have my full focus on the java-agent right now, and won't be able to get around to the scenarios until next week perhaps.

jpkrohling · 2018-10-15T10:03:04Z

Don't worry! I'm starting to check the Katacoda scenarios based on this branch and will send a PR with the required fixes to that repo.

jpkrohling · 2018-10-15T12:37:49Z

java/src/main/java/lesson01/README.md

-    SamplerConfiguration samplerConfig = new SamplerConfiguration("const", 1);
-    ReporterConfiguration reporterConfig = new ReporterConfiguration(true, null, null, null, null);
-    Configuration config = new Configuration(service, samplerConfig, reporterConfig);
+    SamplerConfiguration samplerConfig = new SamplerConfiguration().withType("const").withParam(1);


Could you change this to use fromEnv(), like what we are doing in the Katacoda scenario? We override some env vars there, so, fromEnv() is required there (even though it's not required here).

https://github.com/katacoda-scenarios/opentracing-scenarios/blob/master/opentracing-tutorial-lesson01/03-initialize-real-tracer.md

jpkrohling · 2018-10-15T12:50:39Z

java/src/main/java/lesson01/README.md

-    <version>0.26.0</version>
+    <groupId>io.jaegertracing</groupId>
+    <artifactId>jaeger-client</artifactId>
+    <version>0.31.0</version>


0.32.0, otherwise, you'll need to add the #close() calls. The 0.32.0 implements the "close on JVM shutdown" feature.

@pavolloffay, is 0.32.0 available on maven central already?

it should be, if it's not I don't want to redo the release 😢

It's there, just confirmed :)

Suggested change

<version>0.31.0</version>

<version>0.32.0</version>

jpkrohling · 2018-10-18T09:47:40Z

Could we aim to get this merged by tomorrow, preferably today? I'm OK with the changes here, even though I'd prefer to have 0.32.0 instead of 0.31.0 and omit the tracer.close() calls, but those can be done at a follow up PR.

Once this here is merged, I'll ask @BenHall to merge katacoda-scenarios/opentracing-scenarios#3

yurishkuro · 2018-10-18T21:14:53Z

@jpkrohling you have merge writes here

jpkrohling · 2018-10-19T07:59:42Z

That's true :-) Merging!

safris added 3 commits September 6, 2018 23:17

Resolve deprecation & try-resource warnings

82c9b53

Add required modules to support jdk9+

15419f9

Update lesson[124] README.md

560464c

yurishkuro reviewed Sep 6, 2018

View reviewed changes

jpkrohling reviewed Sep 7, 2018

View reviewed changes

Upgrade to io.jaegertracing:jaeger-core:0.31.0

06b33b4

jpkrohling reviewed Sep 7, 2018

View reviewed changes

safris added 2 commits September 7, 2018 17:53

Make solutions OT vendor agnostic

b748e2f

Replace jaeger-core with jaeger-thrift

06c8847

Replace jaeger-thrift with jaeger-client

77377a2

yurishkuro reviewed Sep 7, 2018

View reviewed changes

java/src/main/java/lesson03/exercise/Hello.java Show resolved Hide resolved

Make solutions OT vendor agnostic

8af3052

jpkrohling reviewed Sep 27, 2018

View reviewed changes

Upcast to JaegerTracer for Closeable

ea77699

jpkrohling mentioned this pull request Oct 16, 2018

Updated tutorial to latest versions katacoda-scenarios/opentracing-scenarios#3

Merged

jpkrohling reviewed Oct 16, 2018

View reviewed changes

jpkrohling merged commit fcf83f0 into yurishkuro:master Oct 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update lessons #30

Update lessons #30

safris commented Sep 6, 2018

yurishkuro Sep 6, 2018

safris Sep 7, 2018

jpkrohling Sep 7, 2018

yurishkuro Sep 6, 2018

safris Sep 7, 2018

jpkrohling Sep 7, 2018

jpkrohling Sep 7, 2018

safris Sep 7, 2018

jpkrohling Sep 7, 2018

jpkrohling Oct 15, 2018

safris commented Sep 7, 2018

jpkrohling Sep 7, 2018

jpkrohling Sep 7, 2018

safris Sep 7, 2018

safris commented Sep 7, 2018 •

edited

Loading

yurishkuro commented Sep 7, 2018

yurishkuro commented Sep 7, 2018

safris commented Sep 8, 2018

jpkrohling commented Sep 10, 2018 •

edited

Loading

safris commented Sep 10, 2018

jpkrohling commented Sep 27, 2018

jpkrohling Sep 27, 2018

safris Sep 27, 2018

jpkrohling commented Sep 28, 2018 •

edited

Loading

safris commented Oct 4, 2018

jpkrohling commented Oct 15, 2018

jpkrohling Oct 15, 2018

jpkrohling Oct 15, 2018

jpkrohling Oct 16, 2018

pavolloffay Oct 16, 2018

jpkrohling Oct 18, 2018

jpkrohling commented Oct 18, 2018

yurishkuro commented Oct 18, 2018

jpkrohling commented Oct 19, 2018

Update lessons #30

Update lessons #30

Conversation

safris commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safris commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safris commented Sep 7, 2018 • edited Loading

yurishkuro commented Sep 7, 2018

yurishkuro commented Sep 7, 2018

safris commented Sep 8, 2018

jpkrohling commented Sep 10, 2018 • edited Loading

safris commented Sep 10, 2018

jpkrohling commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Sep 28, 2018 • edited Loading

safris commented Oct 4, 2018

jpkrohling commented Oct 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Oct 18, 2018

yurishkuro commented Oct 18, 2018

jpkrohling commented Oct 19, 2018

safris commented Sep 7, 2018 •

edited

Loading

jpkrohling commented Sep 10, 2018 •

edited

Loading

jpkrohling commented Sep 28, 2018 •

edited

Loading