feat(tracer): implement span processor #149

mayurkale22 · 2019-07-31T18:56:00Z

This is very big PR (sorry for that), but I felt this was important to connect most of the pieces together like SpanProcessor, BatchSampledSpanProcessor, SimpleSampledSpanProcessor etc.

*Tests are still pending, but I would like to get initial feedback before moving forward (either I will split into smaller PRs or update this PR).

mayurkale22 · 2019-07-31T18:59:00Z

packages/opentelemetry-basic-tracer/src/export/SpanProcessor.ts

+ * SpanProcessor is the interface Tracer SDK uses to allow synchronous hooks
+ * for when a {@link Span} is started or when a {@link Span} is ended.
+ */
+export interface SpanProcessor {


Already done in separate PR #136, please add comments there (if any).

mayurkale22 · 2019-07-31T18:59:39Z

packages/opentelemetry-basic-tracer/src/BasicTracer.ts

-      spanId,
-    });
+    options.parent = parentSpanContext;
+    const span = new Span(this, name, options, this._spanProcessor);


Already done in separate PR #145, please add comments there (if any).

mayurkale22 · 2019-07-31T20:37:02Z

packages/opentelemetry-basic-tracer/src/export/ReadableSpan.ts

+  Event,
+} from '@opentelemetry/types';
+
+export interface ReadableSpan {


This is added in #150, please comment there.

packages/opentelemetry-basic-tracer/src/Span.ts

packages/opentelemetry-basic-tracer/src/export/BatchSampledSpanProcessor.ts

packages/opentelemetry-basic-tracer/src/BasicTracer.ts

hekike · 2019-08-01T13:45:35Z

packages/opentelemetry-basic-tracer/src/Span.ts

@@ -65,6 +68,7 @@ export class Span implements types.Span {
    }
    this._kind = options.kind || types.SpanKind.INTERNAL;
    this._startTime = options.startTime || performance.now();
+    spanProcessor.onStart(this);


if we already have the reference to the tracer should we do this._tracer.spanProcessor(this)?

_spanProcessor is a private member in the tracer class. Need to make it public in order to achieve this.

hekike · 2019-08-01T13:48:38Z

packages/opentelemetry-basic-tracer/src/export/BatchSampledSpanProcessor.ts

+  private _bufferTimeoutInProgress = false;
+  private _spansDataList: ReadableSpan[] = [];
+
+  constructor(private readonly exporter: SpanExporter, config: BufferConfig) {


I'm slightly confused here about the tracer-processor-exporter relationship. If the tracer has one processor and the processor has ONE exporter, how you can have multiple exporters? Shouldn't the processor work with multiple exporters?

I'm also confused about exactly what a processor is supposed to be in this case and the relationship with the exporter. I would think they are unrelated concepts, and vendors should be able to add any number of processors and/or exporters independently.

If the tracer has one processor and the processor has ONE exporter, how you can have multiple exporters? Shouldn't the processor work with multiple exporters?

Makes sense to me, I have changed the SpanProcessor to work with multiple exporters.

vendors should be able to add any number of processors and/or exporters independently.

I have exposed registerExporter() method on tracer to add any number of exporters and for now, SpanProcessor is default to BatchSampledSpanProcessor. Maybe later will allow vendors/users to configure SpanProcessor also. WDYT?

Why is the processor even needed? What is the purpose of the processor?

packages/opentelemetry-basic-tracer/src/platform/node/timer-util.ts

packages/opentelemetry-basic-tracer/src/platform/browser/timer-util.ts

codecov-io · 2019-08-03T03:14:57Z

Codecov Report

Merging #149 into master will decrease coverage by 11.72%.
The diff coverage is 72.92%.

@@             Coverage Diff             @@
##           master     #149       +/-   ##
===========================================
- Coverage   91.95%   80.23%   -11.73%     
===========================================
  Files          48       57        +9     
  Lines        1405     1523      +118     
  Branches       96      109       +13     
===========================================
- Hits         1292     1222       -70     
- Misses        113      301      +188

Impacted Files	Coverage Δ
...kages/opentelemetry-basic-tracer/test/Span.test.ts	`0% <0%> (-100%)`	⬇️
...ry-basic-tracer/src/platform/browser/timer-util.ts	`0% <0%> (ø)`
...ic-tracer/src/export/SimpleSampledSpanProcessor.ts	`100% <100%> (ø)`
...metry-basic-tracer/src/platform/node/timer-util.ts	`100% <100%> (ø)`
...acer/test/export/BatchSampledSpanProcessor.test.ts	`100% <100%> (ø)`
...ntelemetry-basic-tracer/src/platform/node/index.ts	`100% <100%> (ø)`
...s/opentelemetry-basic-tracer/src/platform/index.ts	`100% <100%> (ø)`
packages/opentelemetry-basic-tracer/src/Span.ts	`50% <100%> (-35.72%)`	⬇️
...emetry-basic-tracer/src/export/NoopSpanExporter.ts	`100% <100%> (ø)`
...ages/opentelemetry-basic-tracer/src/BasicTracer.ts	`9.09% <7.69%> (-72.73%)`	⬇️
... and 16 more

draffensperger · 2019-08-07T01:54:06Z

packages/opentelemetry-basic-tracer/test/export/BatchSampledSpanProcessor.test.ts

+      assert.strictEqual(processor['_spansDataList'].length, 0);
+    });
+
+    // it('should force flush when timeout exceeded', done => {


Why is this commented out? If it doesn't work yet, would you be open to just having a @todo comment and create an issue for it? (I'm a bit wary of checking in commented out code since it might not be clear to future people why it's there)

mayurkale22 · 2019-08-07T23:02:48Z

Will revise this PR based on specs decision: open-telemetry/opentelemetry-specification#205. Feel free to add comments there.

mayurkale22 · 2019-08-14T22:32:39Z

Per SIG discussion, please add comments on open-telemetry/opentelemetry-specification#205 to shape SpanProcessor in the right direction.

mayurkale22 · 2019-09-10T21:38:56Z

Closing this now, handled this in the multiple separate PRs.

mayurkale22 requested review from danielkhan, iredelmeier, justindsmith and rochdev as code owners July 31, 2019 18:56

mayurkale22 requested review from hekike and bg451 July 31, 2019 18:56

mayurkale22 commented Jul 31, 2019

View reviewed changes

mayurkale22 mentioned this pull request Jul 31, 2019

feat(sdk): add ReadableSpan #150

Merged

mayurkale22 commented Jul 31, 2019

View reviewed changes

mayurkale22 added the Discussion Issue or PR that needs/is extended discussion. label Jul 31, 2019

mayurkale22 added this to the SDK complete milestone Jul 31, 2019

draffensperger suggested changes Aug 1, 2019

View reviewed changes

packages/opentelemetry-basic-tracer/src/Span.ts Outdated Show resolved Hide resolved

packages/opentelemetry-basic-tracer/src/Span.ts Outdated Show resolved Hide resolved

packages/opentelemetry-basic-tracer/src/export/BatchSampledSpanProcessor.ts Outdated Show resolved Hide resolved

hekike reviewed Aug 1, 2019

View reviewed changes

mayurkale22 force-pushed the span_processor_and_exporter branch from 73c394e to 4c208e3 Compare August 2, 2019 19:13

open-telemetry deleted a comment from codecov-io Aug 2, 2019

draffensperger reviewed Aug 3, 2019

View reviewed changes

packages/opentelemetry-basic-tracer/src/platform/node/timer-util.ts Show resolved Hide resolved

draffensperger reviewed Aug 3, 2019

View reviewed changes

packages/opentelemetry-basic-tracer/src/platform/browser/timer-util.ts Outdated Show resolved Hide resolved

mayurkale22 force-pushed the span_processor_and_exporter branch from 49331a9 to ef28b4e Compare August 3, 2019 03:13

draffensperger approved these changes Aug 3, 2019

View reviewed changes

This was referenced Aug 3, 2019

Implement console exporter #120

Closed

feat(plugin): add http plugin #161

Merged

mayurkale22 force-pushed the span_processor_and_exporter branch from ef28b4e to c039d0d Compare August 6, 2019 17:46

open-telemetry deleted a comment from codecov-io Aug 6, 2019

mayurkale22 force-pushed the span_processor_and_exporter branch from c039d0d to 4b09b68 Compare August 6, 2019 17:49

draffensperger reviewed Aug 7, 2019

View reviewed changes

mayurkale22 removed the request for review from justindsmith August 7, 2019 21:20

mayurkale22 mentioned this pull request Aug 7, 2019

feat(sdk): add SpanExporter interface #177

Merged

mayurkale22 added the onHold label Aug 9, 2019

mayurkale22 changed the title ~~feat(tracer): add span processor and exporter~~ feat(tracer): implement span processor Aug 9, 2019

mayurkale22 force-pushed the span_processor_and_exporter branch 3 times, most recently from 639648b to a9035ed Compare August 13, 2019 17:58

feat(tracer): add span processor and exporter

ede8972

mayurkale22 force-pushed the span_processor_and_exporter branch from a9035ed to ede8972 Compare August 14, 2019 22:44

mayurkale22 mentioned this pull request Aug 15, 2019

Zipkin Exporter #192

Merged

OlivierAlbertini mentioned this pull request Aug 19, 2019

[http-plugin] Refactor tests once exporter available #209

Closed

mayurkale22 mentioned this pull request Aug 23, 2019

feat: add SimpleSpanProcessor #218

Merged

mayurkale22 mentioned this pull request Sep 6, 2019

feat: add BatchSpanProcessor #238

Merged

mayurkale22 closed this Sep 10, 2019

mayurkale22 deleted the span_processor_and_exporter branch September 10, 2019 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracer): implement span processor #149

feat(tracer): implement span processor #149

mayurkale22 commented Jul 31, 2019 •

edited

Loading

mayurkale22 Jul 31, 2019

mayurkale22 Jul 31, 2019

mayurkale22 Jul 31, 2019

hekike Aug 1, 2019

mayurkale22 Aug 2, 2019

hekike Aug 1, 2019

rochdev Aug 1, 2019

mayurkale22 Aug 6, 2019

rochdev Aug 7, 2019

codecov-io commented Aug 3, 2019 •

edited

Loading

draffensperger Aug 7, 2019

mayurkale22 commented Aug 7, 2019

mayurkale22 commented Aug 14, 2019

mayurkale22 commented Sep 10, 2019

feat(tracer): implement span processor #149

feat(tracer): implement span processor #149

Conversation

mayurkale22 commented Jul 31, 2019 • edited Loading

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

codecov-io commented Aug 3, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mayurkale22 commented Aug 7, 2019

mayurkale22 commented Aug 14, 2019

mayurkale22 commented Sep 10, 2019

mayurkale22 commented Jul 31, 2019 •

edited

Loading

codecov-io commented Aug 3, 2019 •

edited

Loading