You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by pavolloffay <gi...@git.apache.org> on 2017/04/28 12:16:13 UTC
[GitHub] httpclient pull request #74: WIP: OpenTracing integration
GitHub user pavolloffay opened a pull request:
https://github.com/apache/httpclient/pull/74
WIP: OpenTracing integration
OpenTracing integration.
Tracing `ClientExecChain` has to be registered as the last one. It should wrap all the client invocation.
There will be probably one more class - `SpanDecorator` to add tags to span (onRequest, onResponse, onError).
cc @ok2c
(`mvn clean install -Dcheckstyle.skip=true -Dmaven.javadoc.skip=true -Drat.skip=true -DskipTests`)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pavolloffay/httpclient opentracing
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/httpclient/pull/74.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #74
----
commit ba308ed6bcc156433f4e44cae73b3b56295b809e
Author: Pavol Loffay <pl...@redhat.com>
Date: 2017-04-28T12:04:09Z
OpenTracing integration
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #74: WIP: OpenTracing integration
Posted by pavolloffay <gi...@git.apache.org>.
Github user pavolloffay commented on the issue:
https://github.com/apache/httpclient/pull/74
This integration can live in a separate repo, if there will be a way how to add `TracingExec` as the last one in the chain.
Question about async client. Requests probably happen in a different thread. Is there a way how to access code creating those callables/runnables? I will have to do something like this? https://github.com/OpenFeign/feign-opentracing/blob/master/feign-hystrix-opentracing/src/main/java/feign/opentracing/hystrix/TracingConcurrencyStrategy.java#L75
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #74: WIP: OpenTracing integration
Posted by pavolloffay <gi...@git.apache.org>.
Github user pavolloffay commented on the issue:
https://github.com/apache/httpclient/pull/74
Agree OT integration should optional. I'm just trying to make sure that integration is possible for both clients.
Take your time. Ping me please when it is ready.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient pull request #74: WIP: OpenTracing integration
Posted by pavolloffay <gi...@git.apache.org>.
Github user pavolloffay commented on a diff in the pull request:
https://github.com/apache/httpclient/pull/74#discussion_r116216114
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/TracingAsyncExec.java ---
@@ -0,0 +1,80 @@
+package org.apache.hc.client5.http.impl.async;
+
+import java.io.IOException;
+import java.net.URISyntaxException;
+
+import org.apache.hc.client5.http.async.AsyncExecCallback;
+import org.apache.hc.client5.http.async.AsyncExecChain;
+import org.apache.hc.client5.http.async.AsyncExecChain.Scope;
+import org.apache.hc.client5.http.async.AsyncExecChainHandler;
+import org.apache.hc.core5.http.EntityDetails;
+import org.apache.hc.core5.http.HttpException;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.nio.AsyncDataConsumer;
+import org.apache.hc.core5.http.nio.AsyncEntityProducer;
+
+import io.opentracing.Span;
+import io.opentracing.Tracer;
+import io.opentracing.Tracer.SpanBuilder;
+import io.opentracing.contrib.spanmanager.DefaultSpanManager;
+import io.opentracing.contrib.spanmanager.SpanManager.ManagedSpan;
+import io.opentracing.tag.Tags;
+
+/**
+ * @author Pavol Loffay
+ */
+public class TracingAsyncExec implements AsyncExecChainHandler {
+
+ private Tracer tracer;
+
+ public TracingAsyncExec(Tracer tracer) {
+ this.tracer = tracer;
+ }
+
+ @Override
+ public void execute(HttpRequest request, AsyncEntityProducer entityProducer, final Scope scope,
+ AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) throws HttpException, IOException {
+
+ SpanBuilder spanBuilder = tracer.buildSpan(request.getMethod())
+ .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT);
+
+ ManagedSpan current = DefaultSpanManager.getInstance().current();
--- End diff --
@ok2c I need your look here.
Does this always run in the same thread as `client.execute()`?
`current` is stored in thread local before calling the client.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #74: WIP: OpenTracing integration
Posted by adriancole <gi...@git.apache.org>.
Github user adriancole commented on the issue:
https://github.com/apache/httpclient/pull/74
as much as I like tracing, I also think it would be extremely unwise to add more deps to the bottom of the stack (which is often where httpclient is), particularly in v0 apis that have few java developers reviewing them
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient pull request #74: WIP: OpenTracing integration
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpclient/pull/74#discussion_r116219416
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/TracingAsyncExec.java ---
@@ -0,0 +1,80 @@
+package org.apache.hc.client5.http.impl.async;
+
+import java.io.IOException;
+import java.net.URISyntaxException;
+
+import org.apache.hc.client5.http.async.AsyncExecCallback;
+import org.apache.hc.client5.http.async.AsyncExecChain;
+import org.apache.hc.client5.http.async.AsyncExecChain.Scope;
+import org.apache.hc.client5.http.async.AsyncExecChainHandler;
+import org.apache.hc.core5.http.EntityDetails;
+import org.apache.hc.core5.http.HttpException;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.nio.AsyncDataConsumer;
+import org.apache.hc.core5.http.nio.AsyncEntityProducer;
+
+import io.opentracing.Span;
+import io.opentracing.Tracer;
+import io.opentracing.Tracer.SpanBuilder;
+import io.opentracing.contrib.spanmanager.DefaultSpanManager;
+import io.opentracing.contrib.spanmanager.SpanManager.ManagedSpan;
+import io.opentracing.tag.Tags;
+
+/**
+ * @author Pavol Loffay
+ */
+public class TracingAsyncExec implements AsyncExecChainHandler {
+
+ private Tracer tracer;
+
+ public TracingAsyncExec(Tracer tracer) {
+ this.tracer = tracer;
+ }
+
+ @Override
+ public void execute(HttpRequest request, AsyncEntityProducer entityProducer, final Scope scope,
+ AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) throws HttpException, IOException {
+
+ SpanBuilder spanBuilder = tracer.buildSpan(request.getMethod())
+ .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT);
+
+ ManagedSpan current = DefaultSpanManager.getInstance().current();
--- End diff --
No, it does not. It can also happen on a I/O dispatch thread. If you want pass bits of contextual data to the request executors you should be using HttpContext associated with the request.
PS: We are migrating from SVN to Git. I'll likely take a few more days before I can merge your pull requests. Please bear with me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #74: WIP: OpenTracing integration
Posted by adriancole <gi...@git.apache.org>.
Github user adriancole commented on the issue:
https://github.com/apache/httpclient/pull/74
Oleg, thinking in general here.. I think one hook approach that would
be nice to see in any instrumentation library is an interface like
this..
Scope newScope(Attributes attributes);
which is used in try-finally form whenever user interceptors are invoked.
try (Scope wa = newScope(attributes) {
userInterceptorChain.invoke();
}
this approach was discussed here:
https://github.com/openzipkin/brave/issues/166#issuecomment-277634798
An implementation of this scope could be picking trace context out of
the attributes and setting it in a thread local. closing the scope
removes it. This is cheaper than trying to do the same with lambdas,
and also keeps the call stack less complex
As different users may pick different things, a composition of scopes
would be inside the "newScope" method above. It might have to act like
guava's Closer such that all scopes are closed even if one causes a
problem.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #74: WIP: OpenTracing integration
Posted by adriancole <gi...@git.apache.org>.
Github user adriancole commented on the issue:
https://github.com/apache/httpclient/pull/74
hey, oleg. different project but same problem.
Here's an example of instrumenting the async client w/ v4.3
(apologies I made no useful code comments explaining..)
https://github.com/openzipkin/brave/blob/master/instrumentation/httpasyncclient/src/main/java/brave/httpasyncclient/TracingHttpAsyncClientBuilder.java
The main concerns are split into a several areas:
* how to propagate state internally throughout the request (ex stuffing
things in attributes)
* how to make state visible to user interceptors (ex thread scoping)
* how to ensure span lifecycle happens (callbacks)
* what to add to the span (parsing)
You can do everything except the "user interceptors" part and tracing will
work fine. It is just that some users want to do things like add to a trace
manually. That's what's lost when you don't have clean hooks into executors
or other means to start work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient pull request #74: WIP: OpenTracing integration
Posted by pavolloffay <gi...@git.apache.org>.
Github user pavolloffay commented on a diff in the pull request:
https://github.com/apache/httpclient/pull/74#discussion_r116227875
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/TracingAsyncExec.java ---
@@ -0,0 +1,80 @@
+package org.apache.hc.client5.http.impl.async;
+
+import java.io.IOException;
+import java.net.URISyntaxException;
+
+import org.apache.hc.client5.http.async.AsyncExecCallback;
+import org.apache.hc.client5.http.async.AsyncExecChain;
+import org.apache.hc.client5.http.async.AsyncExecChain.Scope;
+import org.apache.hc.client5.http.async.AsyncExecChainHandler;
+import org.apache.hc.core5.http.EntityDetails;
+import org.apache.hc.core5.http.HttpException;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.nio.AsyncDataConsumer;
+import org.apache.hc.core5.http.nio.AsyncEntityProducer;
+
+import io.opentracing.Span;
+import io.opentracing.Tracer;
+import io.opentracing.Tracer.SpanBuilder;
+import io.opentracing.contrib.spanmanager.DefaultSpanManager;
+import io.opentracing.contrib.spanmanager.SpanManager.ManagedSpan;
+import io.opentracing.tag.Tags;
+
+/**
+ * @author Pavol Loffay
+ */
+public class TracingAsyncExec implements AsyncExecChainHandler {
+
+ private Tracer tracer;
+
+ public TracingAsyncExec(Tracer tracer) {
+ this.tracer = tracer;
+ }
+
+ @Override
+ public void execute(HttpRequest request, AsyncEntityProducer entityProducer, final Scope scope,
+ AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) throws HttpException, IOException {
+
+ SpanBuilder spanBuilder = tracer.buildSpan(request.getMethod())
+ .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT);
+
+ ManagedSpan current = DefaultSpanManager.getInstance().current();
--- End diff --
This makes tracing really difficult for users.
Is there a way how to instrument executors used by the client?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient pull request #74: WIP: OpenTracing integration
Posted by adriancole <gi...@git.apache.org>.
Github user adriancole commented on a diff in the pull request:
https://github.com/apache/httpclient/pull/74#discussion_r114044778
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/impl/sync/HttpClientBuilder.java ---
@@ -888,6 +898,10 @@ public void close() throws IOException {
closeablesCopy.add(connManagerCopy);
}
+ if (tracer != null) {
+ execChain = new TracingExec(execChain, tracer, DefaultSpanManager.getInstance());
--- End diff --
some hook to do the same is the key. I also noticed the redirect problem (which could be ok as in okhttp we define an application span which makes a child for each network attempt)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient pull request #74: WIP: OpenTracing integration
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpclient/pull/74#discussion_r116229419
--- Diff: httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/TracingAsyncExec.java ---
@@ -0,0 +1,80 @@
+package org.apache.hc.client5.http.impl.async;
+
+import java.io.IOException;
+import java.net.URISyntaxException;
+
+import org.apache.hc.client5.http.async.AsyncExecCallback;
+import org.apache.hc.client5.http.async.AsyncExecChain;
+import org.apache.hc.client5.http.async.AsyncExecChain.Scope;
+import org.apache.hc.client5.http.async.AsyncExecChainHandler;
+import org.apache.hc.core5.http.EntityDetails;
+import org.apache.hc.core5.http.HttpException;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.nio.AsyncDataConsumer;
+import org.apache.hc.core5.http.nio.AsyncEntityProducer;
+
+import io.opentracing.Span;
+import io.opentracing.Tracer;
+import io.opentracing.Tracer.SpanBuilder;
+import io.opentracing.contrib.spanmanager.DefaultSpanManager;
+import io.opentracing.contrib.spanmanager.SpanManager.ManagedSpan;
+import io.opentracing.tag.Tags;
+
+/**
+ * @author Pavol Loffay
+ */
+public class TracingAsyncExec implements AsyncExecChainHandler {
+
+ private Tracer tracer;
+
+ public TracingAsyncExec(Tracer tracer) {
+ this.tracer = tracer;
+ }
+
+ @Override
+ public void execute(HttpRequest request, AsyncEntityProducer entityProducer, final Scope scope,
+ AsyncExecChain chain, final AsyncExecCallback asyncExecCallback) throws HttpException, IOException {
+
+ SpanBuilder spanBuilder = tracer.buildSpan(request.getMethod())
+ .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT);
+
+ ManagedSpan current = DefaultSpanManager.getInstance().current();
--- End diff --
1. I may be missing something obvious but what is so difficult about shoving things into HttpContext?
2. Async client has a different threading model compared to the classic one thread per socket one, like it or not.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #74: WIP: OpenTracing integration
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/74
@pavolloffay I am still working on a redesign of request execution pipeline in my private branch (both classic and async). It will likely take me another week or two to complete. Please just bear with me.
I am not sure I am in favor of adding OpenTracing as a hard dependency to HttpClient. I personally would rather prefer it to be a separate module like HTTP cache, Windows integration and OSGi.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #74: WIP: OpenTracing integration
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpclient/pull/74
@adriancole Adrain, feel to propose or, better, to contribute "clean hooks into executors or other means to start work" to HttpClient 5.0. There we can still shape APIs as we deem fit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpclient issue #74: WIP: OpenTracing integration
Posted by adriancole <gi...@git.apache.org>.
Github user adriancole commented on the issue:
https://github.com/apache/httpclient/pull/74
>
> @adriancole <https://github.com/adriancole> Adrain, feel to propose or,
> better, to contribute "clean hooks into executors or other means to start
> work" to HttpClient 5.0. There we can still shape APIs as we deem fit.
>
I'll try to instrument what's already in 5 and then see how it goes (I
won't be using code in this PR). I'll run through brave's test suites and
see how it goes and let you know!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org