Add a new module supporting jaeger - opentracing #101

cmoulliard · 2017-06-06T15:13:20Z

Added a new maven module supporting jaeger - opentracing
Added a starter to deploy a Spring Boot Jaeger application
Project has been tested using the ola spring boot application

codefromthecrypt · 2017-06-06T15:26:11Z

spring-cloud-kubernetes-dependencies/pom.xml

@@ -20,6 +20,9 @@
 		<mockwebserver.version>0.0.12</mockwebserver.version>
 		<restassured.version>3.0.2</restassured.version>
 		<spock-spring.version>1.0-groovy-2.3</spock-spring.version>
+		<version.jaeger>0.19.0</version.jaeger>
+		<io.opentracing.version>0.22.0</io.opentracing.version>


fyi 0.22 is api incompatible with 0.30

This is fine here, these versions match

I will update the version as jaeger-core - 0.19.0 has a dep with opentracing 0.22

[INFO] com.uber.jaeger:jaeger-core:jar:0.19.0 [INFO] +- com.uber.jaeger:jaeger-thrift:jar:0.19.0:compile [INFO] | \- org.apache.thrift:libthrift:jar:0.9.2:compile [INFO] | +- org.apache.httpcomponents:httpclient:jar:4.2.5:compile [INFO] | | +- commons-logging:commons-logging:jar:1.1.1:compile [INFO] | | \- commons-codec:commons-codec:jar:1.6:compile [INFO] | \- org.apache.httpcomponents:httpcore:jar:4.2.4:compile [INFO] +- io.opentracing:opentracing-api:jar:0.22.0:compile [INFO] +- com.google.code.gson:gson:jar:2.8.0:compile [INFO] +- org.slf4j:slf4j-api:jar:1.7.16:compile

codefromthecrypt · 2017-06-06T15:35:26Z

my 2p: this seems like more a community project than something we'd have in spring-cloud-kubernetes. sleuth is the spring-cloud's tracing solution. We don't want to inherit maintenance for third party apis, particularly api unstable ones never requested by an end user

pavolloffay · 2017-06-07T07:56:16Z

...in/java/org/springframework/cloud/kubernetes/jaeger/KubernetesJaegerDiscoveryProperties.java

+@ConfigurationProperties("spring.cloud.kubernetes.jaeger.discovery")
+public class KubernetesJaegerDiscoveryProperties {
+
+	private String tracerServerName = "jaeger-all-in-one";


Should be called agentServiceName data could be sent to agent and collector.

Will do that.

pavolloffay · 2017-06-07T08:03:46Z

...main/java/org/springframework/cloud/kubernetes/jaeger/JaegerKubernetesAutoConfiguration.java

+		URL url = new URL(serviceURL);
+		Sender sender = new UdpSender(url.getHost(), url.getPort(), 0);
+		return new com.uber.jaeger.Tracer.Builder(serviceName,
+			new RemoteReporter(sender, 100, 50,


Some of these properties should be configurable, see https://github.com/uber/jaeger-client-java/tree/master/jaeger-core#configuration-via-environment at least queue size and flush interval.

pavolloffay · 2017-06-07T08:07:10Z

slueth is not OpenTracing compatible and we would like to go that direction.

codefromthecrypt · 2017-06-07T08:12:47Z

slueth is not OpenTracing compatible and we would like to go that direction.

feel free to host your own integration. If users ask about it, we can mention how to find yours!

codefromthecrypt · 2017-06-07T08:15:22Z

+ <version.jaeger>0.19.0</version.jaeger> + <io.opentracing.version>0.22.0</io.opentracing.version> This is fine here, these versions match

it isn't fine. the point is that this api breaks constantly and will introduce thrash. bringing unstable components into a repo isn't something to take lightly.

codefromthecrypt · 2017-06-07T08:20:18Z

There's community effort (by a user of sleuth) here https://github.com/DealerDotCom/sleuth-opentracing if opentracing becomes stable and/or activity picks up there, we can consider formalizing. Adding this to the core repo ahead of demand and with known unstable apis is just not a right time thing

cmoulliard · 2017-06-07T09:41:48Z

it isn't fine. the point is that this api breaks constantly and will
introduce thrash. bringing unstable components into a repo isn't something
to take lightly.

It's true about api breaks (like it has also been the case for AMQP or WebSocket till 1.0 have been released). However, the OT API version 1.0 should be out soon (although the date is not settled) and will stabilize existing situation. Those breaking changes happen because of major new features like active span in-process propagation.

Sleuth combines a variety of instrumentations and is harder to plug in instrumentation for a different framework, on the other hand, pure OT instrumentation allows us easily plug in different instrumentation together.

So, if you refuse this PR because the OT Api isn't stable enough, I will argue you that you have implemented a Spring Boot starter for AMQP - 0.9 while the spec wasn't stable enough. This is not because a spec API is not yet stable that we should not propose/provide an implementation as proposed here based on the Uber Jaeger project which is not at all a "community project" as you suggest but a Microservices Distributed Tracing Project [1] [2] offering more features than Zipkin

Moreover, as an API adapter has been proposed to the OpenTracing foundation [3] for spring Cloud Sleuth, that makes a lot of sense to continue to develop this PR, module. Do you agree ?

[1] : https://eng.uber.com/distributed-tracing/
[2] : https://uber.github.io/jaeger/
[3] : opentracing-contrib/meta#5

codefromthecrypt · 2017-06-07T09:45:41Z

I'd recommend closing this issue

cmoulliard · 2017-06-07T09:52:02Z

I'd recommend closing this issue

Why ?

codefromthecrypt · 2017-06-07T09:53:21Z

The "community project" reference was not a qualitative reference about jaeger. It was that you can absolutely host this integration on your own, as a person in a community, in jaeger's org or opentracing's org or somewhere else.

I'm not going to restate the myriad of reasons why it is not the right time, both from a stability pov, or a relevance POV to put something like this in the core repo!

codefromthecrypt · 2017-06-07T10:27:27Z

ps I'm not a maintainer of this project, so if contributors here decide to merge and maintain this ahead of user demand etc, that's its prerogative. I'm only commenting as a couple people were confused by this out-of-the-blue PR

iocanel · 2017-06-07T10:54:25Z

I will not comment about api stability, as it refers to things I am not familiar with.

I am however, familiar with which integration, should be hosted where kind of talks. So I'll comment on that:

I don't see why we can't host this module here, same way we do for ribbon, hystrix, zipkin.
I also don't see why a feature has to be requested by a user first (especially for an incubating project).

Now, if spring-cloud has a policy like: "we host only one solution per problem (e.g. for tracing we will only host sleuth based stuff)", then I guess as a project that aspires to graduate, we will have to respect that.

cmoulliard · 2017-06-07T11:15:00Z

This PR offers the possibility for Spring project running on Google Kubernetes or Openshift platform to really benefit about a modern OpenTracing API, where instrumentation exists, is independent of the implementation, is vendor neutral.

If you choose zipkin you have to stick with zipkin, OT allows you to change tracing backend with literally zero configuration change. There is no tracer API in zipkin - e.g. zipkin java tracer (brave) has totally different API that tracer in ruby or python.

Because OpenTracing is vendor neutral it is possible (and it is a goal of OT) is to write instrumentation only once. OpenTracing instrumentation can be reused for Zipkin or Jaeger.

The PR module proposed here could be turned into zipkin with minimal changes only tracer creation.

So far all tracing system had to write instrumentation by themselves, the same over and over (instana, datadog, new relic...). OT allows you to write it only once and then focus on thighs which really matters.

So why opentracing --> http://opentracing.io/documentation/#why-opentracing

maxromanovsky · 2017-06-07T11:17:27Z

@cmoulliard I don't get why it's k8s-specific. What about consul or netflix users? Also, IMHO there would be little to no people to support it, and it doesn't belong to k8s repo.

spencergibb · 2017-06-07T11:27:13Z

It's been marked for team discussion, which we will discuss tomorrow.

codefromthecrypt · 2017-06-07T11:42:11Z

@cmoulliard I've read the website and the tag line. Show me the tests that prove any of this works in practice. You do know stagemonitor needed to write a shim to work with two opentracing implementations. Now, they are rev-locked.. until everyone updates the same version.

What you are doing now is wasting more time, and honestly I'm done responding to your trolls.

spencergibb · 2017-06-07T11:45:51Z

I'll reopen discussion after our meeting tomorrow

spencergibb · 2017-06-13T18:12:37Z

Thanks for the PR. There are a few things we'll comment on:

This is not the appropriate project for this PR. We’re focusing our tracing efforts in the Spring Cloud Sleuth project: https://github.com/spring-cloud/spring-cloud-sleuth.
There have been no requests for specific Jaeger support. (Also, Jaeger specifically supports Zipkin spans: http://jaeger.readthedocs.io/en/latest/getting_started/#migrating-from-zipkin.)
On sleuth specifically (because this isn't the sleuth repo), we don't have demand to justify a strict dependency on OpenTracing. We would prefer that exposing OpenTracing apis optionally within Sleuth start as a community project, such as the existing OpenTracing integration for Spring Cloud Sleuth (https://github.com/DealerDotCom/sleuth-opentracing). We feel that we should wait to incorporate this support into a core project. We can revisit the topic once the community repository is more popular and apis are stable.

cmoulliard added 5 commits June 5, 2017 17:14

Add jaeger module

07ccf32

Add missing files

60f969b

Apply patch to create correctly the NoopTracer. Add Logger

3ddf954

Extract IP and PORT

049945a

Optimize imports

857f1c0

spencergibb added the for team discussion label Jun 6, 2017

codefromthecrypt reviewed Jun 6, 2017

View reviewed changes

pavolloffay reviewed Jun 7, 2017

View reviewed changes

spring-cloud locked and limited conversation to collaborators Jun 7, 2017

spencergibb closed this Jun 13, 2017

spring-cloud unlocked this conversation Jun 13, 2017

spencergibb mentioned this pull request Jun 13, 2017

Develop a new module around Jaeger & OpenTracing #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new module supporting jaeger - opentracing #101

Add a new module supporting jaeger - opentracing #101

cmoulliard commented Jun 6, 2017

codefromthecrypt Jun 6, 2017

pavolloffay Jun 7, 2017

cmoulliard Jun 7, 2017

codefromthecrypt commented Jun 6, 2017

pavolloffay Jun 7, 2017

cmoulliard Jun 7, 2017

pavolloffay Jun 7, 2017

cmoulliard Jun 7, 2017

pavolloffay commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017 via email

codefromthecrypt commented Jun 7, 2017 via email

codefromthecrypt commented Jun 7, 2017 via email

cmoulliard commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017

cmoulliard commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017

iocanel commented Jun 7, 2017

cmoulliard commented Jun 7, 2017

maxromanovsky commented Jun 7, 2017

spencergibb commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017

spencergibb commented Jun 7, 2017

spencergibb commented Jun 13, 2017

Add a new module supporting jaeger - opentracing #101

Add a new module supporting jaeger - opentracing #101

Conversation

cmoulliard commented Jun 6, 2017

codefromthecrypt Jun 6, 2017

Choose a reason for hiding this comment

pavolloffay Jun 7, 2017

Choose a reason for hiding this comment

cmoulliard Jun 7, 2017

Choose a reason for hiding this comment

codefromthecrypt commented Jun 6, 2017

pavolloffay Jun 7, 2017

Choose a reason for hiding this comment

cmoulliard Jun 7, 2017

Choose a reason for hiding this comment

pavolloffay Jun 7, 2017

Choose a reason for hiding this comment

cmoulliard Jun 7, 2017

Choose a reason for hiding this comment

pavolloffay commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017 via email

codefromthecrypt commented Jun 7, 2017 via email

codefromthecrypt commented Jun 7, 2017 via email

cmoulliard commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017

cmoulliard commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017

iocanel commented Jun 7, 2017

cmoulliard commented Jun 7, 2017

maxromanovsky commented Jun 7, 2017

spencergibb commented Jun 7, 2017

codefromthecrypt commented Jun 7, 2017

spencergibb commented Jun 7, 2017

spencergibb commented Jun 13, 2017