Add support for websockets #53

objectiser · 2017-09-19T15:35:34Z

Fixes #25

pavolloffay · 2017-09-19T15:44:35Z

...cloud-starter/src/test/java/io/opentracing/contrib/spring/cloud/websocket/WebsocketTest.java

@@ -0,0 +1,74 @@
+/*
+ * Copyright 2016-2017 The OpenTracing Authors


@objectiser could you please create a PR for checking license headers?

thanks 👍

pavolloffay

Just some minor comments. Could you please rebase it? I will try it in the real app.

pavolloffay · 2017-09-20T16:01:47Z

opentracing-spring-cloud-starter/pom.xml

@@ -121,6 +121,16 @@
          <artifactId>artemis-jms-server</artifactId>
          <scope>test</scope>
        </dependency>
+        <dependency>


Why not this:

<dependency> <groupId>org.springframework.boot</groupId> <artifactId>spring-boot-starter-websocket</artifactId> </dependency>

prefer to be consistent with the other pom

pavolloffay · 2017-09-20T16:05:06Z

...d/src/main/java/io/opentracing/contrib/spring/cloud/websocket/TracingChannelInterceptor.java

+        if (SimpMessageType.MESSAGE.equals(message.getHeaders().get(SIMP_MESSAGE_TYPE))) {
+            SpanContext context = null;
+            // Check if server kind
+            if (spanKind == Tags.SPAN_KIND_SERVER) {


use .equals

pavolloffay · 2017-09-20T16:08:28Z

...-cloud/src/main/java/io/opentracing/contrib/spring/cloud/websocket/TextMapInjectAdapter.java

+
+/**
+ * A TextMap carrier for use with Tracer.inject() to inject tracing context into Spring messaging
+ * {link {@link MessageHeaders}.


invalid javadoc

pavolloffay · 2017-09-20T16:08:35Z

...cloud/src/main/java/io/opentracing/contrib/spring/cloud/websocket/TextMapExtractAdapter.java

+
+/**
+ * A TextMap carrier for use with Tracer.extract() to extract tracing context from Spring messaging
+ * {link {@link MessageHeaders}.


invalid javadoc

pavolloffay · 2017-09-20T16:09:52Z

...cloud/src/main/java/io/opentracing/contrib/spring/cloud/websocket/TextMapExtractAdapter.java

+
+    public TextMapExtractAdapter(final MessageHeaders headers) {
+        for (Map.Entry<String, Object> entry : headers.entrySet()) {
+            if (entry.getValue() instanceof String) {


I think it's better to get rid of this check and rather call .toString() then adding to the map

pavolloffay · 2017-09-20T16:26:05Z

.../src/test/java/io/opentracing/contrib/spring/cloud/websocket/SpringWebsocketTracingTest.java

+    private int port;
+    private String url;
+
+    private static final String SEND_HELLO_MESSAGE_ENDPOINT = "/app/hello";


move constants to the top

pavolloffay · 2017-09-20T16:27:26Z

...c/test/java/io/opentracing/contrib/spring/cloud/websocket/TracingChannelInterceptorTest.java

+
+    @Test
+    public void testPreSendServerSpan() {
+        MessageBuilder<String> messageBuilder = MessageBuilder.<String>withPayload("Hi")


diamond operator can be used.

pavolloffay · 2017-09-20T16:27:39Z

...c/test/java/io/opentracing/contrib/spring/cloud/websocket/TracingChannelInterceptorTest.java

+
+    @Test
+    public void testPreSendClientSpan() {
+        MessageBuilder<String> messageBuilder = MessageBuilder.<String>withPayload("Hi")


diamond operator

pavolloffay · 2017-09-20T16:29:44Z

.../src/test/java/io/opentracing/contrib/spring/cloud/websocket/SpringWebsocketTracingTest.java

+import io.opentracing.mock.MockTracer;
+
+@RunWith(SpringJUnit4ClassRunner.class)
+@SpringBootTest(classes= {WebSocketConfig.class,GreetingController.class},


Add MockTracingAutoConfiguration to be consistent with other tests,

pavolloffay · 2017-09-20T16:30:57Z

.../src/test/java/io/opentracing/contrib/spring/cloud/websocket/SpringWebsocketTracingTest.java

+    private List<Transport> createTransportClient() {
+        List<Transport> transports = new ArrayList<>(1);
+        transports.add(new WebSocketTransport(new StandardWebSocketClient()));
+        return transports;


nit: Collections.singletonList()

… Tracer bean and find alternative to PostConstruct approach

pavolloffay · 2017-09-22T17:45:47Z

.../src/main/java/io/opentracing/contrib/spring/cloud/websocket/WebsocketAutoConfiguration.java

+    @Autowired
+    Tracer tracer;
+
+    @Autowired(required=false)


This still does not look right (with logic in bean providers). Can we make it declarative on the methods or class? (you can inject directly to the method and make method conditional on a specific bean)

Fixed - I had tried that before, but was using ConditionalOnBean(ExecutorSubscribableChannel.class) which failed - when I switched to using the bean names it worked.

pavolloffay · 2017-09-25T14:02:12Z

.../src/main/java/io/opentracing/contrib/spring/cloud/websocket/WebsocketAutoConfiguration.java

+    Tracer tracer;
+
+    @Bean
+    @ConditionalOnBean(name="clientInboundChannel")


The string seems fragile to me? Isn't it better to use class name?

if they change the name of bean method it will break.

As mentioned #53 (comment) I originally tried with class but didn't work. There are two beans of the same class, so appears in such a situation the condition fails - so didn't register the channel interceptors.

Updated to use a different approach - but had to remove Tracer.class from the ConditionalOnBean, as it only requires one class in the list to match.

Then this should solve it :) https://paste.fedoraproject.org/paste/U0QFVn9BpnBSk3KF6VFoGg/

pavolloffay · 2017-09-25T14:51:40Z

.../src/main/java/io/opentracing/contrib/spring/cloud/websocket/WebsocketAutoConfiguration.java

+    WebSocketMessageBrokerConfigurationSupport config;
+
+    @Bean
+    public TracingChannelInterceptor tracingInboundChannelInterceptor() {


Now it looks much better. This also worked https://paste.fedoraproject.org/paste/YcocqvZsm6TcFwlVBCoqzw/ (similar to sleuth...)

pavolloffay · 2017-09-25T14:54:18Z

...cloud/src/main/java/io/opentracing/contrib/spring/cloud/websocket/TextMapExtractAdapter.java

+ * A TextMap carrier for use with Tracer.extract() to extract tracing context from Spring messaging
+ * {@link MessageHeaders}.
+ *
+ * @see Tracer#extract(Format, Object)


I have commented before: this is an invalid javadoc you have to import format. The same is in inject adapter.

pavolloffay · 2017-09-25T15:47:34Z

One thing I notice is that it creates a client span for each connected client. I don't have a reasonable knowledge in WS to judge whether it is good or bad.

However, from what know about WS is that all clients are connected to one channel and developer is responsible for a proper routing. So am not sure if we want to create a client span for each connected user.

cc @jpkrohling

pavolloffay · 2017-09-25T15:34:34Z

...d/src/main/java/io/opentracing/contrib/spring/cloud/websocket/TracingChannelInterceptor.java

+ * communications using an OpenTracing Tracer.
+ *
+ */
+public class TracingChannelInterceptor extends ChannelInterceptorAdapter implements ExecutorChannelInterceptor {


We should probably set also a component tag.

pavolloffay · 2017-09-25T15:53:37Z

anyways sleuth also creates a client span for each client.

objectiser · 2017-09-25T15:55:36Z

We could always provide a post processor, as in java-metrics, to have a skip pattern for the inbound/outbound connections. But may be create a separate issue for discussion?

pavolloffay · 2017-09-25T15:58:15Z

But may be create a separate issue for discussion?

👍 as you want.

pavolloffay · 2017-09-25T16:03:49Z

...d/src/main/java/io/opentracing/contrib/spring/cloud/websocket/TracingChannelInterceptor.java

+    /**
+     * The span component tag value.
+     */
+    private static final String SPRING_WEBSOCKET = "spring-websocket";


I think just websocket is better. Users would know framework by service name.

jpkrohling · 2017-09-25T16:13:32Z

However, from what know about WS is that all clients are connected to one channel and developer is responsible for a proper routing. So am not sure if we want to create a client span for each connected user.

I'm a bit lost, but assuming that you mean WebSockets in general (not Spring's, not this PR's), then that's not correct. Each client connection is a Session (channel as you called it). The server can certainly keep a list/map of sessions and propagate messages to all clients, but in the end, each client has its own session.

pavolloffay

LGTM, there should be asserts on tags in tests.

pavolloffay · 2017-09-25T16:54:12Z

I was using this https://spring.io/guides/gs/messaging-stomp-websocket/ example

pavolloffay · 2017-09-26T14:00:14Z

One of the further improvements: #58 (just for linking purposes)

objectiser requested a review from pavolloffay September 19, 2017 15:35

pavolloffay reviewed Sep 19, 2017

View reviewed changes

objectiser force-pushed the websocket branch from aef2d4d to 048c640 Compare September 20, 2017 14:27

pavolloffay reviewed Sep 20, 2017

View reviewed changes

objectiser added 4 commits September 22, 2017 16:41

Add support for websockets

ae081d1

Fixed most issues relate to comments - need to clarify conditional on…

5434502

… Tracer bean and find alternative to PostConstruct approach

Move away from PostConstruct and use ConditionalOnBean for tracer

2a95a18

Update based on comment

3a7d63b

objectiser force-pushed the websocket branch from a15ddbc to 3a7d63b Compare September 22, 2017 15:51

pavolloffay reviewed Sep 22, 2017

View reviewed changes

Improve tracer channel interceptor registration

3739204

pavolloffay reviewed Sep 25, 2017

View reviewed changes

alternative approach

4355d8e

pavolloffay reviewed Sep 25, 2017

View reviewed changes

objectiser added 2 commits September 25, 2017 16:15

fix javadoc

c547466

use websocket broker config as parameters

7719d17

pavolloffay reviewed Sep 25, 2017

View reviewed changes

Add component to span tags

8950ebd

pavolloffay reviewed Sep 25, 2017

View reviewed changes

objectiser mentioned this pull request Sep 25, 2017

RFC: Should spans be reported for websocket client connections #58

Open

Rename component value

5074dc2

pavolloffay approved these changes Sep 25, 2017

View reviewed changes

Test tags

d14ff5c

pavolloffay merged commit 701c23a into opentracing-contrib:master Sep 26, 2017

		@@ -0,0 +1,74 @@
		/*
		* Copyright 2016-2017 The OpenTracing Authors

Add support for websockets #53

Add support for websockets #53

Conversation

objectiser commented Sep 19, 2017 • edited by pavolloffay

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Sep 25, 2017

Choose a reason for hiding this comment

pavolloffay commented Sep 25, 2017

objectiser commented Sep 25, 2017 • edited

pavolloffay commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Sep 25, 2017

pavolloffay left a comment

Choose a reason for hiding this comment

pavolloffay commented Sep 25, 2017

pavolloffay commented Sep 26, 2017

objectiser commented Sep 19, 2017 •

edited by pavolloffay

objectiser commented Sep 25, 2017 •

edited