support open-tracing #81

ascrutae · 2017-01-17T09:00:15Z

No description provided.

wu-sheng · 2017-01-17T09:05:01Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/api/Tracing.java

+     *
+     * @return span. if it exists, or null
+     */
+    public static Span getLatestSpan() {


rename to getActiveSpan or getCurrentSpan. There is no guarantee about, the top span of stack, is the latest span.

wu-sheng · 2017-01-17T09:06:56Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/api/Tracing.java

+     *
+     * @param tags tags map
+     */
+    public static void setTags2CurrentSpan(Map<String, String> tags) {


The method should declare as
public static void setTag(Span span, String tagKey, String tagValue)

wu-sheng · 2017-01-17T09:09:04Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/api/Tracing.java

+    }
+
+
+    public static void putBaseSpan(ContextData contextData){


BaseSpan is a strange name. It's something like prevSpanContext, maybe should be a new type, which contains: traceid, parentSpanId. No need be a span.

wu-sheng · 2017-01-17T09:09:27Z

...niffer/skywalking-api/src/main/java/com/a/eye/skywalking/context/CurrentThreadSpanStack.java

+         * This {@link SpanNode} represent the base span of all the next spans. the trace id and
+         * the level id will base on it.
+         */
+        private SpanNode baseSpan = null;


Rename, and add a new type.

RefContext may be a good name.

wu-sheng · 2017-01-17T09:10:32Z

...niffer/skywalking-api/src/main/java/com/a/eye/skywalking/context/CurrentThreadSpanStack.java

@@ -85,6 +105,10 @@ private void listPush(SpanNode spanNode) {
            spans.add(spans.size(), spanNode);
        }

+        private void initBaseSpan(Span baseSpan){


Rename to initRefContext.

wu-sheng · 2017-01-17T09:11:32Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/model/Identification.java

+            return this;
+        }
+
+        public IdentificationBuilder tags(Map<String, String> tags) {


Method should be tag, instead of tags.

wu-sheng · 2017-01-17T09:13:38Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/util/ContextGenerator.java

@@ -42,12 +42,12 @@ private static Span getSpanFromThreadLocal(Identification id) {
        // 2 校验Context，Context是否存在
        if (parentSpan == null) {
            // 不存在，新创建一个Context
-            span = new Span(id.getViewPoint());
+            span = new Span(id.getViewPoint(), id.getStartTime(), id.getTags());


Tags, as a constructor parameters, is an improper thing.
Span should have a method, named tag()

wu-sheng · 2017-01-17T09:18:48Z

.../main/java/com/a/eye/skywalking/toolkit/activation/opentracing/SkyWalkingSpanActivation.java

+                    @Override
+                    public String getMethodsInterceptor() {
+                        return "com.a.eye.skywalking.toolkit.activation.opentracing" +
+                                ".SkywalkingSpanTagsInterceptor";


All interceptor name should refactor:
com.a.eye.skywalking.toolkit.activation.opentracing.SkywalkingSpanTagsInterceptor
refactor to
com.a.eye.skywalking.toolkit.activation.opentracing.span.SetTagInterceptor

All other methods should stay in the same principle.

wu-sheng · 2017-01-17T09:29:37Z

.../java/com/a/eye/skywalking/toolkit/activation/opentracing/SkyWalkingSpanTagsInterceptor.java

+public class SkyWalkingSpanTagsInterceptor implements InstanceMethodsAroundInterceptor {
+    @Override
+    public void beforeMethod(EnhancedClassInstanceContext context, InstanceMethodInvokeContext interceptorContext, MethodInterceptResult result) {
+       Map<String, String> tags = (Map<String, String>) context.get("tags");


tags and something like this, should be a constant.

wu-sheng · 2017-01-17T11:02:30Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/api/Tracing.java

+    /**
+     * Set the tag to span
+     *
+     * @param span


Comments: the span will be tagged with the given key and value pair.

wu-sheng · 2017-01-17T11:02:42Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/api/Tracing.java

+
+
+    /**
+     * Get the latest span of current trace.


Get the current span

wu-sheng · 2017-01-17T11:04:38Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/api/Tracing.java

+     *
+     * @param contextData context data
+     */
+    public static void putRefContext(ContextData contextData){


putRefContext's parameter should be RefContext type. And caller in charge of converting ContextData to RefContext.

putRefContext should be named set... or init.... There is nothing to put here.

wu-sheng · 2017-01-17T11:05:52Z

...niffer/skywalking-api/src/main/java/com/a/eye/skywalking/context/CurrentThreadSpanStack.java

@@ -42,7 +43,20 @@ public static Span pop() {
        return nodes.get().pop();
    }

+    public static void putRefContext(RefContext refContext) {
+        if (nodes.get() == null){


Why need null-point check?

wu-sheng · 2017-01-17T11:08:08Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/model/BaseSpan.java

@@ -0,0 +1,8 @@
+package com.a.eye.skywalking.model;
+
+public class BaseSpan extends Span {


Why still have the BaseSpan?

will delete it

wu-sheng · 2017-01-17T11:22:52Z

.../java/com/a/eye/skywalking/toolkit/activation/opentracing/span/SkyWalkingSpanActivation.java

+
+                    @Override
+                    public String getConstructorInterceptor() {
+                        return "com.a.eye.skywalking.toolkit.activation.opentracing.span.interceptor.SpanConstructorInterceptor";


Not the correct name again. No need of Span prefix, it already existed in package name.
Maybe com.a.eye.skywalking.toolkit.activation.opentracing.span.interceptor.Constructor? or use other principles.

wu-sheng · 2017-01-17T11:23:40Z

...tivation/opentracing/tracer/interceptor/ExtractCrossProcessByteBufferContextInterceptor.java

+import javax.net.ssl.StandardConstants;
+
+/**
+ * Created by wusheng on 2017/1/3.


Comment code by the actual author name.

wu-sheng · 2017-01-17T11:24:03Z

.../activation/opentracing/tracer/interceptor/ExtractCrossProcessTextMapContextInterceptor.java

+import io.opentracing.propagation.TextMap;
+
+/**
+ * Created by wusheng on 2017/1/3.


Comment code by the actual author name.

wu-sheng · 2017-01-17T11:24:37Z

.../activation/opentracing/tracer/interceptor/ExtractCrossProcessTextMapContextInterceptor.java

+            Map.Entry<String, String> entry = iterator.next();
+            if ("SkyWalking-TRACING-NAME".equals(entry.getKey())){
+                try {
+                    Tracing.putBaseSpan(new ContextData(entry.getValue()));


Still putBaseSpan, check your code carefully.

wu-sheng · 2017-01-17T11:24:49Z

.../activation/opentracing/tracer/interceptor/ExtractCrossProcessTextMapContextInterceptor.java

+        Iterator<Map.Entry<String, String>> iterator = textMap.iterator();
+        while (iterator.hasNext()){
+            Map.Entry<String, String> entry = iterator.next();
+            if ("SkyWalking-TRACING-NAME".equals(entry.getKey())){


SkyWalking-TRACING-NAME need be a constant.

wu-sheng · 2017-01-18T02:34:30Z

...niffer/skywalking-api/src/main/java/com/a/eye/skywalking/context/CurrentThreadSpanStack.java

+        /**
+         * This {@link RefContext} contain the trace context from another processor.
+         */
+        private RefContext refContext = null;


This field should set to null, when spans is empty after pop invokes.

If not, maybe cause a leak when the next transaction begins.

wu-sheng · 2017-01-18T02:37:20Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/model/Identification.java

@@ -29,41 +35,63 @@ public String getBusinessKey() {
        return businessKey;
    }

+    public long getStartTime() {
+        if (startTime == 0){
+            return System.currentTimeMillis();


This is an identification, only be used in create span. Why need getStartTime?
And even need this, why not assign this.startTime = 'return-value'?

wu-sheng · 2017-01-18T02:37:38Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/model/Identification.java

+        return startTime;
+    }
+
+    public Map<String, String> getTags() {


Why need get?

wu-sheng · 2017-01-18T02:39:06Z

skywalking-sniffer/skywalking-api/src/main/java/com/a/eye/skywalking/model/Span.java

@@ -166,12 +170,12 @@ public void setBusinessKey(String businessKey) {

    public RequestSpan.Builder buildRequestSpan(RequestSpan.Builder builder) {
        return builder.setTraceId(this.traceId).setParentLevel(this.parentLevel).setLevelId(this.levelId)
-                .setStartDate(this.startDate).setRouteKey(routeKey).putAllTags(tags);
+                .setStartDate(this.startTime).setRouteKey(routeKey).putAllTags(tags);


Still have two method names, StartDate and StartTime, please make sure they are uniform.

wu-sheng · 2017-01-18T02:43:47Z

...e/skywalking/toolkit/activation/opentracing/spanbuilder/OpenTracingLocalBuriedPointType.java

+/**
+ * Created by xin on 2017/1/16.
+ */
+public class OpenTracingLocalBuriedPointType implements IBuriedPointType {


Only a suggestion, refactor class type to enum seems better. Also suggest in other plugins.

wu-sheng

After you test, this will be ready to be merged.

ascrutae · 2017-01-23T07:06:48Z

The visualization is:

wu-sheng · 2017-01-23T07:28:45Z

@rayzhang0603 , in 2.1-2017, we provide opentracing-implemetation tracer. Have tested by using motan's filter, with your source code, since your next version is not released yet.

Seem passed. The visualization is above.

btw @andot , I think this feature will work in hprose-java filter. opentracing-contrib/java-hprose is in releasing.

wu-sheng requested changes Jan 17, 2017

View reviewed changes

wu-sheng requested changes Jan 18, 2017

View reviewed changes

wu-sheng added the enhancement label Jan 18, 2017

wu-sheng added this to the 2.1-2017 milestone Jan 18, 2017

wu-sheng reviewed Jan 19, 2017

View reviewed changes

wu-sheng approved these changes Jan 23, 2017

View reviewed changes

support open-tracing

7bf8942

wu-sheng merged commit e671b73 into apache:master Jan 23, 2017

linliaoy mentioned this pull request Sep 3, 2020

Oap8.1 An OAP node USES ES6.7 to limit the problem #5421

Closed

4 tasks

yuzuofu mentioned this pull request Feb 21, 2023

[Bug] [PHP]php-fpm not working sometimes, and nginx result in 502 #10424

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support open-tracing #81

support open-tracing #81

ascrutae commented Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

ascrutae Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 17, 2017

wu-sheng Jan 18, 2017

wu-sheng Jan 18, 2017

wu-sheng Jan 18, 2017

wu-sheng Jan 18, 2017

wu-sheng Jan 18, 2017

wu-sheng Jan 18, 2017

wu-sheng left a comment

ascrutae commented Jan 23, 2017 •

edited by wu-sheng

wu-sheng commented Jan 23, 2017

		@@ -0,0 +1,8 @@
		package com.a.eye.skywalking.model;

		public class BaseSpan extends Span {

support open-tracing #81

support open-tracing #81

Conversation

ascrutae commented Jan 17, 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

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

wu-sheng left a comment

Choose a reason for hiding this comment

ascrutae commented Jan 23, 2017 • edited by wu-sheng

wu-sheng commented Jan 23, 2017

ascrutae commented Jan 23, 2017 •

edited by wu-sheng