feat: add BatchSpanProcessor #238

mayurkale22 · 2019-09-06T19:04:57Z

Which problem is this PR solving?

This PR is a follow up of feat: add SimpleSpanProcessor #218 and based on the specs Add SDK span processors spec opentelemetry-specification#205

Short description of the changes

Split from feat(tracer): implement span processor #149

OlivierAlbertini

LGTM

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

hekike · 2019-09-09T00:48:52Z

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

+    }
+
+    if (this._bufferTimeoutInProgress) {
+      this._resetBufferTimeout();


shouldn't it be race (max buffer size or timeout)? If we reset the buffer always and we add one span at every 19999ms we won't report for 100 * 19999ms = ~33 mins

Yeah this could be an annoying issue, i think the buffer timeout should be more like a flush ticker, so we end up with behavior where the tracer flushes every N seconds AND flushes when the buffer is at capacity.

+1 to this. Is there a scenario where we would ever want to reset the timer? We would send every N seconds, regardless of when the last span was added to the buffer. If the buffer becomes size M, we flush and clearTimeout.

Do you suggest to remove the reset functionality? If we do that, every span end we are creating a setting new timer. Let me know if you have a better approach to handle this.

Could a timer only be started on the first span to enter the current batch?

What about this:

Every time a span is written we set a variable for "lastSpanWrite" time. Also, if writing a span would fill the buffer, we flush it

We have a regular timer with setInterval that runs and checks if lastSpanWrite was above some time threshold, and if so, it does a write of all spans in the buffer

It's somewhat imperfect as the actual wait time for a span to be written could vary (but is still bounded by timer frequency + allowed span age), but it avoids the issue of re-creating timers for every span write

I have made the changes in 8b362fb, ptal

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

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

bg451 · 2019-09-10T15:06:49Z

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

+    }
+
+    if (this._bufferTimeoutInProgress) {
+      this._resetBufferTimeout();


Yeah this could be an annoying issue, i think the buffer timeout should be more like a flush ticker, so we end up with behavior where the tracer flushes every N seconds AND flushes when the buffer is at capacity.

draffensperger · 2019-09-13T12:58:53Z

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

+    }
+
+    if (this._bufferTimeoutInProgress) {
+      this._resetBufferTimeout();


What about this:

Every time a span is written we set a variable for "lastSpanWrite" time. Also, if writing a span would fill the buffer, we flush it

We have a regular timer with setInterval that runs and checks if lastSpanWrite was above some time threshold, and if so, it does a write of all spans in the buffer

It's somewhat imperfect as the actual wait time for a span to be written could vary (but is still bounded by timer frequency + allowed span age), but it avoids the issue of re-creating timers for every span write

draffensperger · 2019-09-13T12:59:45Z

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

+      }
+    }, this._bufferTimeout);
+    // Don't let this timer be the only thing keeping the process alive
+    unrefTimer(timer);


What happens if a server is shut down gracefully via a drain signal (to finish requests it has but not take any new ones)? Is there a way to guarantee that it flushes out all its spans? (Maybe just a TODO item for now)

codecov-io · 2019-09-15T18:17:49Z

Codecov Report

Merging #238 into master will increase coverage by 0.02%.
The diff coverage is 98.03%.

@@            Coverage Diff            @@
##           master    #238      +/-   ##
=========================================
+ Coverage   98.87%   98.9%   +0.02%     
=========================================
  Files          67      70       +3     
  Lines        2569    2728     +159     
  Branches      172     181       +9     
=========================================
+ Hits         2540    2698     +158     
- Misses         29      30       +1

Impacted Files	Coverage Δ
...etry-basic-tracer/src/export/BatchSpanProcessor.ts	`100% <100%> (ø)`
...ages/opentelemetry-core/src/platform/node/index.ts	`100% <100%> (ø)`	⬆️
...asic-tracer/test/export/BatchSpanProcessor.test.ts	`100% <100%> (ø)`
...opentelemetry-core/src/platform/node/timer-util.ts	`33.33% <33.33%> (ø)`
.../opentelemetry-exporter-jaeger/test/jaeger.test.ts	`100% <0%> (ø)`	⬆️
...entelemetry-exporter-jaeger/test/transform.test.ts	`100% <0%> (ø)`	⬆️
...metry-node-tracer/src/instrumentation/constants.ts	`100% <0%> (ø)`	⬆️
...pe-async-hooks/test/AsyncHooksScopeManager.test.ts	`100% <0%> (ø)`	⬆️
...e-tracer/test/instrumentation/PluginLoader.test.ts	`100% <0%> (ø)`	⬆️
... and 3 more

draffensperger · 2019-09-17T12:54:45Z

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

+
+  /** Add a span in the buffer. */
+  private _addToBuffer(span: ReadableSpan) {
+    this._lastSpanWrite = Date.now();


Should we have this here? I was thinking we would only set this value in flush. Maybe call it this._lastSpanFlush.

Basically we only want to flush spans if it's been a while since the last span was flushed so that a program that only generates a few spans will still get them flushed out within 2x of the buffer flush timeout.

draffensperger · 2019-09-17T12:55:40Z

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

+
+    it('should force flush when timeout exceeded', done => {
+      const processor = new BatchSpanProcessor(exporter, defaultBufferConfig);
+      for (let i = 0; i < defaultBufferConfig.bufferSize; i++) {


Should we do something like .bufferSize - 1 to explicitly show we are putting in less than the full buffer of spans and then seeing that it still gets flushed?

bg451

besides Dave's comment about lastSpan write, lgtm

bg451 · 2019-09-18T23:36:18Z

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

+
+  /** Add a span in the buffer. */
+  private _addToBuffer(span: ReadableSpan) {
+    this._lastSpanWrite = Date.now();


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

…sor.ts Co-Authored-By: Olivier Albertini <[email protected]>

….0-proposal

mayurkale22 requested review from bg451, danielkhan, iredelmeier, OlivierAlbertini and rochdev as code owners September 6, 2019 19:04

OlivierAlbertini approved these changes Sep 8, 2019

View reviewed changes

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

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

hekike suggested changes Sep 9, 2019

View reviewed changes

draffensperger reviewed Sep 9, 2019

View reviewed changes

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

bg451 reviewed Sep 10, 2019

View reviewed changes

mayurkale22 force-pushed the batch_span_processor branch from 167542b to 341b4f7 Compare September 11, 2019 20:07

draffensperger reviewed Sep 13, 2019

View reviewed changes

mayurkale22 force-pushed the batch_span_processor branch from 644f8ff to 8b362fb Compare September 15, 2019 17:59

draffensperger reviewed Sep 17, 2019

View reviewed changes

bg451 approved these changes Sep 18, 2019

View reviewed changes

bg451 reviewed Sep 18, 2019

View reviewed changes

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

OlivierAlbertini reviewed Sep 20, 2019

View reviewed changes

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

mayurkale22 force-pushed the batch_span_processor branch from 8b362fb to 7970467 Compare September 20, 2019 17:53

mayurkale22 requested a review from vmarchaud as a code owner September 20, 2019 17:53

mayurkale22 force-pushed the batch_span_processor branch from ad9ee95 to fe12733 Compare September 23, 2019 22:31

mayurkale22 mentioned this pull request Sep 24, 2019

feat(plugin-http): add example #318

Merged

draffensperger approved these changes Sep 24, 2019

View reviewed changes

mayurkale22 and others added 7 commits September 25, 2019 10:05

feat: add BatchSpanProcessor

28894a8

fix: remove default value, already done Ctor

fce9620

fix: append the MS unit

e6be022

fix: move unrefTimer function in core

cfe0660

fix: use SetInterval instead of SetTimer

9cdad20

fix: rename TraceOptions -> TraceFlags

6bee7df

Update packages/opentelemetry-basic-tracer/src/export/BatchSpanProces…

af24cd2

…sor.ts Co-Authored-By: Olivier Albertini <[email protected]>

mayurkale22 added 2 commits September 25, 2019 10:05

fix: remove _MS

afaddad

fix: rename _lastSpanWrite -> _lastSpanFlush

bed36f8

mayurkale22 force-pushed the batch_span_processor branch from 2664258 to bed36f8 Compare September 25, 2019 17:05

mayurkale22 merged commit cb035ce into open-telemetry:master Sep 25, 2019

mayurkale22 deleted the batch_span_processor branch September 25, 2019 18:09

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

Merge pull request open-telemetry#238 from dynatrace-oss-contrib/0.11…

f4d1a71

….0-proposal

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add BatchSpanProcessor #238

feat: add BatchSpanProcessor #238

mayurkale22 commented Sep 6, 2019

OlivierAlbertini left a comment

hekike Sep 9, 2019

bg451 Sep 10, 2019

markwolff Sep 10, 2019

mayurkale22 Sep 11, 2019 •

edited

Loading

markwolff Sep 12, 2019

draffensperger Sep 13, 2019

mayurkale22 Sep 16, 2019

bg451 Sep 10, 2019

draffensperger Sep 13, 2019

draffensperger Sep 13, 2019

codecov-io commented Sep 15, 2019

draffensperger Sep 17, 2019

bg451 Sep 18, 2019

draffensperger Sep 17, 2019

bg451 left a comment

bg451 Sep 18, 2019

feat: add BatchSpanProcessor #238

feat: add BatchSpanProcessor #238

Conversation

mayurkale22 commented Sep 6, 2019

Which problem is this PR solving?

Short description of the changes

OlivierAlbertini left a 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

mayurkale22 Sep 11, 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

codecov-io commented Sep 15, 2019

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bg451 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 Sep 11, 2019 •

edited

Loading