Add support for child span count #332
Conversation
Codecov Report
@@ Coverage Diff @@
## master #332 +/- ##
==========================================
+ Coverage 94.95% 94.96% +<.01%
==========================================
Files 120 120
Lines 8213 8229 +16
Branches 732 732
==========================================
+ Hits 7799 7815 +16
Misses 414 414
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #332 +/- ##
==========================================
+ Coverage 94.98% 94.99% +<.01%
==========================================
Files 120 120
Lines 8332 8345 +13
Branches 741 741
==========================================
+ Hits 7914 7927 +13
Misses 418 418
Continue to review full report at Codecov.
|
| @@ -169,6 +184,7 @@ describe('RootSpan', () => { | |||
| for (const span of root.spans) { | |||
| assert.ok(span.ended); | |||
| } | |||
| assert.equal(root.numberOfChildren, 1); | |||
There was a problem hiding this comment.
there is no need to check for child count in other test case unless the test case affects the logic.
| @@ -206,6 +222,7 @@ describe('RootSpan', () => { | |||
| rootSpan.addAnnotation('description test', {} as Attributes, Date.now()); | |||
|
|
|||
| assert.ok(rootSpan.annotations.length > 0); | |||
| assert.equal(rootSpan.numberOfChildren, 0); | |||
AFAIK there isn't a notion of grandchild spans in node (all child spans are direct children of a root span). #105 |
mayurkale22
force-pushed
the
child_count
branch
from
4c6cf35
to
67ab8a9
Compare
Is that by design based on the specs / other language implementations? Or is that more of a specific limitation of OpenCensus Node at this time? |
|
@draffensperger allow me to rephrase my previous sentence #332 (comment). AFAIK in terms of single process or server, there will be only two types of requests (incoming and outgoing). Root spans corresponds to incoming requests and child span for outgoing. There is only in and out (no grand out). So, if there are multiple micro services running we connect all these spans through common traceId (which is propagated through context).
I think it makes sense to have child count property of the |
|
I agree that for RPCs you typically only have in and out, but if we allow for other types of spans, I think we can get the grandchild case. I'm thinking of custom intermediate spans that represent larger conceptual things like "process middleware" or "load recommendations" and similar, that each may have multiple RPCs or other custom spans in them. As a user views a trace, they could then see which high level operation in the request is slow and then drill down to the subcalls of that operation. This of course requires that the user sets up those custom spans, but ideally I would think we would let them do that. |
|
I think the work I'm suggesting is captured well in a previous issue that I just commented on: #105 (comment) So I'm fine with merging this and then adding support for nested child spans at a later time. |
Ok cool, thanks. |
Fixes #306
This is based on spec: See https://github.com/census-instrumentation/opencensus-proto/blob/6d79e46232e026136ae013489ef611ca721cb839/src/opencensus/proto/trace/v1/trace.proto#L302.