[GitHub] [skywalking-nodejs] kezhenxu94 commented on a change in pull request #9: [WIP] express plugin, make spans actually work

[GitBox <gi...@apache.org>]

kezhenxu94 commented on a change in pull request #9:
URL: https://github.com/apache/skywalking-nodejs/pull/9#discussion_r536926354



##########
File path: src/trace/context/SpanContext.ts
##########
@@ -57,39 +58,62 @@ export default class SpanContext implements Context {
       });
     }
 
-    ContextManager.spansDup();
+    const spans = ContextManager.spansDup();
+    const parent = spans[spans.length - 1];
 
-    const span = new EntrySpan({
-      id: this.spanId++,
-      parentId: this.parentId,
-      context: this,
-      operation,
-    });
+    let span;
 
-    if (carrier && carrier.isValid()) {
-      span.inject(carrier);
+    if (parent && parent.type === SpanType.ENTRY) {
+      span = parent;
+      parent.operation = operation;
+
+    } else {
+      span = new EntrySpan({
+        id: this.spanId++,
+        parentId: this.parentId,
+        context: this,
+        operation,
+      });
+
+      if (carrier && carrier.isValid()) {
+        span.extract(carrier);
+      }
     }
 
     return span;
   }
 
-  newExitSpan(operation: string, peer: string): Span {
+  newExitSpan(operation: string, peer: string, carrier?: ContextCarrier): Span {
     if (logger.isDebugEnabled()) {
       logger.debug('Creating exit span', {
         parentId: this.parentId,
         executionAsyncId: executionAsyncId(),
       });
     }
 
-    ContextManager.spansDup();
+    const spans = ContextManager.spansDup();
+    const parent = spans[spans.length - 1];
 
-    return new ExitSpan({
-      id: this.spanId++,
-      parentId: this.parentId,
-      context: this,
-      peer,
-      operation,
-    });
+    let span;
+
+    if (parent && parent.type === SpanType.EXIT) {
+      span = parent;
+
+    } else {
+      span = new ExitSpan({
+        id: this.spanId++,
+        parentId: this.parentId,
+        context: this,
+        peer,
+        operation,
+      });
+
+      // if (carrier && carrier.isValid()) {  // is this right?
+      //   Object.assign(carrier, span.inject());

Review comment:
       You don't need to do anything here, context carriers from exit span are meant to be sent to remote peer, so the plugin (or user codes) should tackle this and propagate them to remote peer (via http headers or MQ metadata, etc.) properly, like the http plugin
   
   ```nodejs
   span.inject().items.forEach((item) => {
      request.setHeader(item.key, item.value);
   });
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org