Initial implementation #2

pavolloffay · 2017-04-26T14:13:21Z

No description provided.

jpkrohling · 2017-04-26T14:35:42Z

README.md

+
+```java
+GlogalTracer.register(tracer);
+HttpClient httpClient = new TracingHttpClientBuilder()


Wouldn't it be possible to use a HttpRequestInterceptor, instead of requiring consumers to use this special builder?

It has limitations, it would not finish/mark as error if request results in an error (e.g. unknown host). Which is a big disadvantage. This approach is going to be in brave4 too.

jpkrohling · 2017-04-26T14:38:28Z

README.md

+
+### SpanManager
+```java
+spanManager.activate(parentSpan);


Add a comment with the limitations. For instance, I believe this will not work properly if people are making async calls, as it's then executed in separate threads/span contexts.

This instrumentation does not work for async client.

I mean, a comment on the readme or somewhere the users might read :)

jpkrohling · 2017-04-26T14:54:21Z

...-apache-httpclient/src/main/java/io/opentracing/contrib/apache/TracingHttpClientBuilder.java

+                    if (response != null) {
+                        Integer redirectCount = clientContext.getAttribute(REDIRECT_COUNT, Integer.class);
+                        if (!redirectHandlingDisabled &&
+                                clientContext.getRequestConfig().isRedirectsEnabled() &&


I think this redirect handling logic should be explained in a comment. Like:

For requests that results in redirects, we create child spans for each redirect and call onRedirect, until we reach the redirect limit. Once we reach the limit of redirects, we just call the onResponse and step out.

jpkrohling · 2017-04-26T14:57:29Z

pom.xml

+
+    <version.io.opentracing.contrib-opentracing-spanmanager>0.0.4</version.io.opentracing.contrib-opentracing-spanmanager>
+    <version.io.opentracing>0.21.0</version.io.opentracing>
+    <version.org.apache.httpcomponents-httpclient>4.5.3</version.org.apache.httpcomponents-httpclient>


Would this also work with 5.x?

jpkrohling · 2017-04-26T14:57:54Z

pom.xml

+      <dependency>
+        <groupId>junit</groupId>
+        <artifactId>junit</artifactId>
+        <version>4.12</version>


You externalized all other versions except this one. Is this on purpose?

pavolloffay · 2017-04-27T05:33:39Z

cc @objectiser

objectiser

Only other comment is same as on many initial PRs - better to split out the project infrastructure setup into own PR, so that the initial code PR is simpler.

objectiser · 2017-04-27T07:42:21Z

...apache-httpclient/src/main/java/io/opentracing/contrib/apache/ApacheClientSpanDecorator.java

+
+            Header locationHeader = response.getFirstHeader("Location");
+            if (locationHeader != null) {
+                redirectLogs.put("Location", locationHeader.getValue());


nit: should this be lowercase - seems to be the convention.

May I ask where did you find it?

Couldn't actually find it hence "seems" :) - but from looking at the values that have been defined on https://github.com/opentracing/specification/blob/master/semantic_conventions.md this does appear to be the case - but it should really be defined.

It should be a convention for standard keys to be lower case to easy to use.

objectiser · 2017-04-27T07:48:15Z

...-apache-httpclient/src/main/java/io/opentracing/contrib/apache/TracingHttpClientBuilder.java

+     * @param redirectHandlingDisabled disable redirect strategy, do not call
+     * {@link org.apache.http.impl.client.HttpClientBuilder#setRedirectStrategy(RedirectStrategy)}
+     */
+    public TracingHttpClientBuilder(RedirectStrategy redirectStrategy,


Why is this not using the builder pattern when called Builder - and setting up the items using with... methods?
Assuming something related to inheriting from HttpClientBuilder - if so, might be better to wrap instead of inherit to provide a cleaner builder?

The problem is that builder methods are final 😞 and builder itself is not an interface 😞

Would it be worth having a separate builder then that wraps - does it matter that it is not 'implementing' a httpclientbuilder?

Actually another consideration (not for this PR) is how an agent rule would instrument an app that is using the standard HttpClientBuilder - may require some enhancements here at some point.

Good point about agent 👍 better would be to register ClientExecChain as the last one in the builder. Now it is an anonymous class, we could declare it as a normal class which could be used by the agent.

TracingExec has been moved to a separate class to use it in agent

codefromthecrypt · 2017-04-29T03:15:47Z

actually the wrapping approach here is also similarly used in one of the other opentracing libraries as well https://github.com/lucidsoftware/opentracing-httpcomponents

Might be worth making a registry of things like this, jaxrs, and servlet which have several implementations in opentracing and mildly different semantics

codefromthecrypt · 2017-04-29T03:18:27Z

...apache-httpclient/src/main/java/io/opentracing/contrib/apache/ApacheClientSpanDecorator.java

+        }
+
+        @Override
+        public void onRedirect(HttpResponse response, HttpContext httpContext, Span span) {


note you might actually consider a redirect (or rather any network attempt) a different span. notably as the uri socket or even tls might be different. in okhttp brave we use a separate span to represent the application request and one for each network attempt. I haven't done this in brave4 yet as the main thing there was general api style (not nitty gritty stuff like redirects or requests served from cache)

pavolloffay · 2017-05-04T08:39:50Z

@adriancole thanks for comments!

I re-designed it with local and client span for each network attempt. It looks good. Previously I used logs just for simplicity.

objectiser

The package is too general - io.opentracing.contrib.apache - needs to be specific to httpclient.

Would it be worth waiting a couple of days and replace use of SpanManager with the new active span mechanism - based on Ben's latest comment it sounds like it will be merged tomorrow. This would enable removing the manual span passing aswell.

objectiser · 2017-05-04T10:41:23Z

...tracing-apache-httpclient/src/main/java/io/opentracing/contrib/apache/TracingClientExec.java

+        return (response = handleNetworkProcessing(localSpan, route, request, clientContext, execAware));
+      }
+
+      localSpan = handleLocalSpan(request, clientContext);


Instead of code above, why not just surround this statement by if (localSpan == null) ?

objectiser · 2017-05-04T10:47:09Z

...tracing-apache-httpclient/src/main/java/io/opentracing/contrib/apache/TracingClientExec.java

+  }
+
+  protected Span handleLocalSpan(HttpRequest httpRequest, HttpClientContext clientContext) {
+    Tracer.SpanBuilder spanBuilder = tracer.buildSpan(httpRequest.getRequestLine().getMethod())


Not sure about creating the local span - I can see a possible benefit if there is a redirect, otherwise it appears (correct me if wrong) that a non-redirected invocation will now result in two spans - one internal and one client span?

Another option would be to just identify reference from subsequent client spans to the span they are redirected from - which could be a reusable type that could also be used in fallback/retry situations.

Related to: opentracing/specification#67

You are correct this instrumentation will produce always >= 2 spans.

Generally speaking, there are more ways how to model this. It also highly depends on the hooks what a given framework provides. What also plays a role is the size of created tracing data. There are pros and cons for both, however, I think the most important is that timing data and references are correct.

I was thinking about one span + additional spans for redirects, but there is a problem how spans are referenced:

If there is a redirect the first span should contain tags related to the first request(so it represents first request by data and timing information). Then after response we know that there is a redirect, so we know there will be next span, we also know that we should finish the first span because it's end of that execution chain. Then If we immediately create a new span at the end of chain timing related to the previous span is different (assumption that for the same operation span is created at the same place and wraps execution of the same code). If we wait and create redirect span at the beginning of the chain we can use only followsFrom because the previous span has been finished.

Couldn't the SpanContext of the redirected span be cached somewhere, so that when the redirected invoke is performed, it would just setup the reference? In terms of reference type, it would need to be something new, indicating that this span is a replacement/fallback/etc (good name to be found) for the referenced span.

Any object can be stored in context attributes.

In terms of reference type, it would need to be something new, indicating that this span is a replacement/fallback/etc (good name to be found) for the referenced span.

this does not seem right to me. I'm not sure how far should reference types go. e.g. compared to relationships in UML..

A reference can represent any type of association between two spans. The proposed reference is simply to indicate that one span is taking over for another for some reason (i.e. redirection, failure, retry, etc). This is what I was hoping to discuss on the spec issue - to gain some opinions. So feel free to comment there and we get hopefully get other inputs.

objectiser · 2017-05-04T10:50:48Z

...-apache-httpclient/src/main/java/io/opentracing/contrib/apache/TracingHttpClientBuilder.java

+
+    @Override
+    protected ClientExecChain decorateProtocolExec(final ClientExecChain requestExecutor) {
+        GlobalTracer.register(tracer);


The framework integration shouldn't register the tracer - this needs to be done in a single place within the app ideally.

thanks I forgot this when quickly moving ClientExec to a separate class.

pavolloffay · 2017-05-04T11:31:56Z

@objectiser I would like to keep manual passing of parent span for backup use cases when span management cannot be used. Move to new OT API can be done as an incremental change. This will be still under 0.x

jpkrohling · 2017-05-04T12:04:25Z

README.md

+
+### SpanManager
+```java
+spanManager.activate(parentSpan);


I mean, a comment on the readme or somewhere the users might read :)

jpkrohling · 2017-05-04T12:07:34Z

...client/src/main/java/io/opentracing/contrib/apache/http/client/HttpHeadersInjectAdapter.java

+/**
+ * @author Pavol Loffay
+ */
+class HttpHeadersInjectAdapter implements TextMap {


Is this really only supposed to be used by classes on the same package? I just came across a situation where I needed to use the InjectAdapter from the JMS integration into my own code, so, it might be a good idea to make this public.

We can make it public

jpkrohling · 2017-05-04T12:11:56Z

...client/src/main/java/io/opentracing/contrib/apache/http/client/TracingHttpClientBuilder.java

+/**
+ * @author Pavol Loffay
+ */
+public class TracingHttpClientBuilder extends org.apache.http.impl.client.HttpClientBuilder {


You import this class already, why are you using the fully qualified class name?

pavolloffay · 2017-05-04T13:09:35Z

@jpkrohling @objectiser @adriancole thanks for review!

Initial implementation

aec97a1

pavolloffay requested a review from jpkrohling April 26, 2017 14:13

jpkrohling reviewed Apr 26, 2017

View reviewed changes

objectiser approved these changes Apr 27, 2017

View reviewed changes

pavolloffay added 7 commits April 27, 2017 13:26

Move TracingExec to a separate class

f23371e

Add maven wrapper

4c44d29

Add travis secrets

4f97b27

maven release settings

363ee97

travis publish script

da20a7e

internal comment about redirects

8052065

Move PARENT_SPAN to constants

86c9e16

codefromthecrypt reviewed Apr 29, 2017

View reviewed changes

objectiser mentioned this pull request Apr 29, 2017

Redirection, fault tolerance, circuit breaker, etc patterns opentracing/specification#67

Open

pavolloffay added 4 commits May 3, 2017 17:35

Use local span and client span

79bec04

Internal refactoring

e98e279

Bump OT to 0.20.0

d4f2541

externalize junit version

6b4e847

make Constants constructor private

793517c

objectiser reviewed May 4, 2017

View reviewed changes

pavolloffay added 2 commits May 4, 2017 13:19

Fix review comments

44c5e3e

Change package to .apache.http.client

8c3a5ee

jpkrohling approved these changes May 4, 2017

View reviewed changes

Juca review comments

5ff5ff5

pavolloffay merged commit 6595086 into opentracing-contrib:master May 4, 2017

Initial implementation #2

Initial implementation #2

Conversation

pavolloffay commented Apr 26, 2017

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

pavolloffay commented Apr 27, 2017

objectiser left a 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

Choose a reason for hiding this comment

codefromthecrypt commented Apr 29, 2017

Choose a reason for hiding this comment

pavolloffay commented May 4, 2017

objectiser left a 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

Choose a reason for hiding this comment

pavolloffay commented May 4, 2017

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

pavolloffay commented May 4, 2017