fix(tracer): report every span to exporters #493
Conversation
hekike
requested review from
draffensperger,
justindsmith and
mayurkale22
as code owners
hekike
force-pushed
the
fix/tracer-exporter-span-notifier
branch
3 times, most recently
from
9884134
to
70dc167
Compare
|
Hmm, both tests and code coverage succeed locally. |
hekike
force-pushed
the
fix/tracer-exporter-span-notifier
branch
from
70dc167
to
359e3c2
Compare
| @@ -155,7 +155,7 @@ describe('Zipkin Exporter', function() { | |||
| 'localEndpoint': {'serviceName': 'opencensus-tests'}, | |||
| 'name': 'spanTest', | |||
| 'parentId': rootSpan.id, | |||
| 'shared': true, | |||
| 'shared': false, | |||
There was a problem hiding this comment.
Add a test for when shared should be true.
There was a problem hiding this comment.
Currently, it has one bigger test that contains both root and child span translation and covers both cases of shared. shared is not the only difference, but I guess we could break it down to separate root and child span tests.
| root.end(); | ||
| assert.strictEqual(rootSpanVerifier.endedRootSpans.length, 1); | ||
| // 5 child spand + root span ended |
| @@ -80,7 +80,7 @@ export class ExporterBuffer { | |||
| */ | |||
| addToBuffer(root: modelTypes.Span) { | |||
There was a problem hiding this comment.
s/root/span, also update the param comment?
| @@ -41,10 +41,10 @@ describe('Span', () => { | |||
| /** | |||
| * Should create a span | |||
| */ | |||
| describe('new Span()', () => { | |||
| describe('new Span(tracer, )', () => { | |||
There was a problem hiding this comment.
Nit: remove comma or add span param.
| @@ -101,6 +101,11 @@ export class JaegerTraceExporter implements Exporter { | |||
| * @param root the ended span | |||
| */ | |||
| onEndSpan(root: Span) { | |||
| @@ -155,7 +155,7 @@ export class ZipkinTraceExporter implements Exporter { | |||
| * @param span Span to be translated | |||
| */ | |||
| translateSpan(span: Span): TranslatedSpan { | |||
| const spanTraslated = { | |||
| const spanTranslated = { | |||
| onEndSpan(root: Span) { | ||
| this.endedRootSpans.push(root); | ||
| onEndSpan(span: Span) { | ||
| // TODO(hekike): Tests depends on the order of recorded spans. |
There was a problem hiding this comment.
If possible, please create an issue to track this later.
There was a problem hiding this comment.
Good idea, added: #501
|
Please fix the build |
hekike
force-pushed
the
fix/tracer-exporter-span-notifier
branch
from
fac2ccc
to
0d560ab
Compare
Codecov Report
@@ Coverage Diff @@
## master #493 +/- ##
==========================================
- Coverage 94.97% 94.65% -0.32%
==========================================
Files 145 145
Lines 10208 10237 +29
Branches 874 876 +2
==========================================
- Hits 9695 9690 -5
- Misses 513 547 +34
Continue to review full report at Codecov.
|
|
Thanks, CI fixed |
|
I will merge this PR later today if no other comment(s) from @draffensperger |
Today Tracer only reports root spans to exporters.
After this change every span will be reported to exporters, letting exporters filter on what they want to handle.
Today some exporters (
opencensus-exporter-jaeger) walk throughrootSpan.spans, while others (opencensus-exporter-ocagent) useadaptRootSpan()to flatten spans, while other exporters completely skip child spans.This PR doesn't change Jaeger and ocagent exporters behaviour.
Changes:
opencensus-exporter-jaegerto match the previous behaviouropencensus-exporter-ocagentto match the previous behavioursharedproperty for child spansI learned yesterday from @bogdandrutu that Java reports all the spans to exporters.