Add tracing for entity reading and writing. #46

hypnoce · 2017-10-22T20:37:13Z

Hi all,

I added reader and writer interceptors to measure serialization time.
The code of the client and server are almost the same. I can factorise them if needs be.
Let me know if you have any comment on the feature itself and on the implementation. I will be happy to hear them.

Thanks

pavolloffay · 2017-10-24T12:00:48Z

@hypnoce hi, thanks for a nice contribution!

I have a couple of general comments:

client and server interceptors could be unified, we would have just one tracing interceptor (for both client and server).
decorators for operation names are redundant. Default operation names could be hardcoded as serialize and deserialize respectively.

hypnoce · 2017-10-24T12:52:48Z

No problem, I will make the needed changes when I'm out of meetings :)

pavolloffay

It looks good. We need to change reference types to be semantically correct.

pavolloffay · 2017-10-24T07:45:12Z

.../java/io/opentracing/contrib/jaxrs2/itest/common/AbstractServerDefaultConfigurationTest.java

@@ -39,7 +39,7 @@ public void testDefaultOperationNameAndTags() {
                .get();
        response.close();

-        Assert.assertEquals(1, mockTracer.finishedSpans().size());
+        Assert.assertEquals(2, mockTracer.finishedSpans().size());


Could you please also test the second span?

pavolloffay · 2017-10-24T12:34:38Z

...test/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractClientTest.java

+    }
+
+    private void assertSerializationSpan(MockSpan parentSpan, MockSpan serializationSpan) {
+        Assert.assertTrue(Objects.toString(serializationSpan.tags().get("media.type")).contains(MediaType.TEXT_PLAIN));


Assert.equals is better here. The same for server tests.

Resteasy client returns the charset. Will make sure to remove it before assertion.

pavolloffay · 2017-10-24T12:39:32Z

...-itest/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/rest/TestHandler.java

+    @Path("/postWithBody")
+    @Produces(MediaType.TEXT_PLAIN)
+    @Consumes(MediaType.TEXT_PLAIN)
+    public Response postWithBody(@Context HttpServletRequest request, String body) throws ExecutionException, InterruptedException {


I think we can have only this endpoint and remove putWithBody and get

pavolloffay · 2017-10-24T12:41:26Z

...test/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractServerTest.java

+
+    @Test
+    public void testSerializationResponseAndRequestWithBody() {
+        String response = client.target(url("/postWithBody"))


Could you please use ClientBuilder.newClient() here? There is already a test for testing if wiring works properly.

pavolloffay · 2017-10-24T12:57:55Z

...test/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractClientTest.java

+        List<MockSpan> mockSpans = mockTracer.finishedSpans();
+        Assert.assertEquals(2, mockSpans.size());
+
+        final MockSpan serializationSpan = mockSpans.get(1);


Client requests

Serialization of request body happens between the tracing filter invocation so we can use child_of.

Deserialization happens after the request is processed by the client filter therefore we cannot use child_of relationship. We should change this to follows_from see https://github.com/opentracing/specification/blob/master/specification.md#references-between-spans

Server requests

Deserialization happens between the span in the server filter is started and finished so we can use child_of

Serialization of response entity happens after the server span if finished so we can use only follows_from. (a note for me: If we integrate with servlet filter Integrate with servlet filter #13 then the wrapping span would be always finished after writing the entity).

For more see: https://jersey.github.io/documentation/latest/user-guide.html#d0e9613

If we are going to unify interceptors then we have to do:

use the same name for span wrappers

check if span wrapper has been finished or not when starting a serialization/deserialization span

pavolloffay · 2017-10-24T13:06:37Z

...test/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractServerTest.java

@@ -27,7 +30,7 @@ public void testServerStandardTags() throws Exception {
            .get();
        response.close();

-        Assert.assertEquals(1, mockTracer.finishedSpans().size());
+        Assert.assertEquals(2, mockTracer.finishedSpans().size());


Could you please remove server entities from /clientTracingChaining and "/hello/{id} so the tests remain as they are?

hypnoce · 2017-10-30T09:17:05Z

@pavolloffay I made the changes according to your helpful comments. Let me know if there is anything else.

pavolloffay

Look good just a couple of comments.

pavolloffay · 2017-10-30T09:48:51Z

...src/main/java/io/opentracing/contrib/jaxrs2/serialization/SerdeInterceptorSpanDecorator.java

+import io.opentracing.BaseSpan;
+import javax.ws.rs.ext.InterceptorContext;
+
+public interface SerdeInterceptorSpanDecorator {


I think there is a typo Serde

pavolloffay · 2017-10-30T09:49:10Z

...axrs2/src/main/java/io/opentracing/contrib/jaxrs2/serialization/SerdeTracingInterceptor.java

+import javax.ws.rs.ext.WriterInterceptor;
+import javax.ws.rs.ext.WriterInterceptorContext;
+
+public abstract class SerdeTracingInterceptor implements WriterInterceptor, ReaderInterceptor {


a typo Serde

Name just TracingInterceptor would work.

pavolloffay · 2017-10-30T09:49:49Z

...axrs2/src/main/java/io/opentracing/contrib/jaxrs2/serialization/SerdeTracingInterceptor.java

+        if(spanWrapper == null) {
+            return NoopActiveSpanSource.NoopActiveSpan.INSTANCE;
+        }
+        final Tracer.SpanBuilder serdeSpanBuilder = tracer.buildSpan(operationName);


typo serde?

pavolloffay · 2017-10-30T09:53:09Z

...src/main/java/io/opentracing/contrib/jaxrs2/serialization/SerdeInterceptorSpanDecorator.java

+    /**
+     * Set static serialization operation names.
+     */
+    SerdeInterceptorSpanDecorator SERIALIZE_OPERATION_NAME = new SerdeInterceptorSpanDecorator() {


We don't need this decorator, it uses the same operation names as hardcoded ones.

pavolloffay · 2017-10-30T09:56:50Z

...jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerSerdeTracingInterceptor.java

+     * Use the current HttpServletRequest to lookup the current span wrapper.
+     */
+    @Context
+    private HttpServletRequest servletReq;


:) we see more differences between impls

pavolloffay · 2017-10-30T11:40:03Z

...axrs2/src/main/java/io/opentracing/contrib/jaxrs2/serialization/SerdeTracingInterceptor.java

+            try {
+                return context.proceed();
+            } catch (Exception e) {
+                Tags.ERROR.set(activeSpan, true);


Could you please add a todo here

// Add exception logs in case they are not added by the filter.

hypnoce · 2017-10-31T23:06:28Z

@pavolloffay I used 'serde' as a convention when dealing with serialisation and deserialisation classes. I saw this in kafka I think :). Removed.
I made the changes. Let me know if that looks fine ! Thanks

hypnoce · 2017-11-02T08:29:37Z

@pavolloffay I just noticed something : the follows_from reference is never used. Indeed, in case of a synchronous request, the spanWrapper object is never finished.

Therefore, when checking for completion in TracingInterceptor, the spanWrapper is not finished but the actual underlying span is. Also, in the mock tracing library, there is no simple way to check for follows_from reference type as it is added as a normal parent.

pavolloffay · 2017-11-02T08:38:58Z

I will have a look at this PR today.

Also, in the mock tracing library, there is no simple way to check for follows_from reference type as it is added as a normal parent.

agree mock tracer does not support it, either multiple references. I will create an issue in upstream.

pavolloffay · 2017-11-02T10:36:18Z

MockTracer issue opentracing/opentracing-java#210.

@hypnoce you are right

java-jaxrs/opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/server/ServerTracingFilter.java

Line 103 in c443ad9

if (activeSpan != null) {

is a bug for sync requests the wrapper is not finished.

I will do a fix PR and then retest this.

hypnoce · 2017-11-02T10:56:35Z

@pavolloffay I made a fix on my side. But hard to test unless I had a filter to get the spanWrapper and check if it's finished.

pavolloffay · 2017-11-02T12:55:45Z

The fix is here #47

hypnoce · 2017-11-02T14:18:05Z

Rebased. I still cannot test for follows_from. Do you want to wait until we can properly validate this ?

pavolloffay · 2017-11-02T14:50:43Z

I think it will take a longer time to get it implemented in OT API. We can create an issue to fix it :)

pavolloffay

Just a minor comments, It looks great!

pavolloffay · 2017-11-03T12:26:49Z

opentracing-jaxrs2/src/main/java/io/opentracing/contrib/jaxrs2/internal/SpanWrapper.java

@@ -10,6 +10,8 @@
 */
 public class SpanWrapper {

+    public static final String SPAN_PROP_ID = SpanWrapper.class.getName() + ".activeSpanWrapper";


We can change it to PROPERTY_NAME.

pavolloffay · 2017-11-03T12:28:44Z

...test/common/src/main/java/io/opentracing/contrib/jaxrs2/itest/common/AbstractServerTest.java

@@ -78,13 +81,15 @@ public void testAsyncSever() throws Exception {
        response.close();

        List<MockSpan> mockSpans = mockTracer.finishedSpans();
-        Assert.assertEquals(2, mockSpans.size());
+        Assert.assertEquals(3, mockSpans.size());


Could you please change /async endpoint to not return any value so the interceptor does not kick in.

hypnoce · 2017-11-05T16:22:34Z

@pavolloffay made changes according to your last comments. Added a PR for exposing reference type from MockSpan : opentracing/opentracing-java#213

pavolloffay · 2017-11-06T08:27:57Z

@hypnoce thanks. I will merge and create an issue to test it.

hypnoce changed the title ~~A tracing for entity reading and writing.~~ Add tracing for entity reading and writing. Oct 22, 2017

hypnoce force-pushed the serialization_tracing branch 4 times, most recently from cebdfa4 to ea0e6d9 Compare October 22, 2017 22:18

pavolloffay reviewed Oct 24, 2017

View reviewed changes

hypnoce force-pushed the serialization_tracing branch 4 times, most recently from 00f96d1 to 2cd62a2 Compare October 30, 2017 09:15

pavolloffay reviewed Oct 30, 2017

View reviewed changes

hypnoce force-pushed the serialization_tracing branch from 2cd62a2 to acbd6a3 Compare October 31, 2017 23:10

pavolloffay mentioned this pull request Nov 2, 2017

Finish span wrapper #47

Merged

hypnoce force-pushed the serialization_tracing branch from acbd6a3 to 8e700ef Compare November 2, 2017 14:17

pavolloffay reviewed Nov 3, 2017

View reviewed changes

A tracing for entity reading and writing.

38efd07

hypnoce force-pushed the serialization_tracing branch from 8e700ef to 38efd07 Compare November 5, 2017 15:48

pavolloffay merged commit 1062ce5 into opentracing-contrib:master Nov 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracing for entity reading and writing. #46

Add tracing for entity reading and writing. #46

hypnoce commented Oct 22, 2017

pavolloffay commented Oct 24, 2017

hypnoce commented Oct 24, 2017

pavolloffay left a comment

pavolloffay Oct 24, 2017

pavolloffay Oct 24, 2017

hypnoce Oct 29, 2017

pavolloffay Oct 24, 2017

pavolloffay Oct 24, 2017

pavolloffay Oct 24, 2017

pavolloffay Oct 24, 2017

hypnoce commented Oct 30, 2017

pavolloffay left a comment

pavolloffay Oct 30, 2017

pavolloffay Oct 30, 2017

pavolloffay Oct 30, 2017

pavolloffay Oct 30, 2017

pavolloffay Oct 30, 2017

pavolloffay Oct 30, 2017

pavolloffay Oct 30, 2017

hypnoce commented Oct 31, 2017 •

edited

Loading

hypnoce commented Nov 2, 2017

pavolloffay commented Nov 2, 2017

pavolloffay commented Nov 2, 2017

hypnoce commented Nov 2, 2017

pavolloffay commented Nov 2, 2017 •

edited

Loading

hypnoce commented Nov 2, 2017

pavolloffay commented Nov 2, 2017

pavolloffay left a comment

pavolloffay Nov 3, 2017

pavolloffay Nov 3, 2017

hypnoce commented Nov 5, 2017

pavolloffay commented Nov 6, 2017

Add tracing for entity reading and writing. #46

Add tracing for entity reading and writing. #46

Conversation

hypnoce commented Oct 22, 2017

pavolloffay commented Oct 24, 2017

hypnoce commented Oct 24, 2017

pavolloffay 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

Client requests

Server requests

Choose a reason for hiding this comment

hypnoce commented Oct 30, 2017

pavolloffay 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

hypnoce commented Oct 31, 2017 • edited Loading

hypnoce commented Nov 2, 2017

pavolloffay commented Nov 2, 2017

pavolloffay commented Nov 2, 2017

hypnoce commented Nov 2, 2017

pavolloffay commented Nov 2, 2017 • edited Loading

hypnoce commented Nov 2, 2017

pavolloffay commented Nov 2, 2017

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypnoce commented Nov 5, 2017

pavolloffay commented Nov 6, 2017

hypnoce commented Oct 31, 2017 •

edited

Loading

pavolloffay commented Nov 2, 2017 •

edited

Loading