tracing: Fixes issue with small LightStep reports. by rnburn · Pull Request #3989 · envoyproxy/envoy

rnburn · 2018-07-30T17:41:09Z

Description:
Changes to use a larger default size for LightStep reports sent to satellites. Fixes lightstep/lightstep-tracer-cpp#106

Risk Level: Low

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn · 2018-07-30T17:42:01Z

cc @jmacd

ccaraman · 2018-07-30T18:37:33Z

@rnburn Please fix tests and ping once ready for a review.

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn · 2018-07-30T20:27:32Z

@ccaraman - should be ready for review now.

ccaraman · 2018-08-01T01:15:09Z

source/extensions/tracers/lightstep/lightstep_tracer_impl.cc

-        [this] { return runtime_.snapshot().getInteger("tracing.lightstep.min_flush_spans", 5U); }};
+
+    tls_options.max_buffered_spans = std::function<size_t()>{[this] {
+      return runtime_.snapshot().getInteger("tracing.lightstep.min_flush_spans",


Will there be any issues experienced by users when switching to this higher value?

We (LightStep) are currently experiencing issues resulting from excessively small Envoy reports. Envoy will consume more memory buffering requests, but have less overhead sending small RPCs to LightStep satellites.

There isn't an ideal setting here, but the current default is an order of magnitude or two out of line with what we'd like.

ccaraman · 2018-08-01T01:17:34Z

source/extensions/tracers/lightstep/lightstep_tracer_impl.cc

+// creates separate tracers for each thread.
+//
+// See https://github.com/lightstep/lightstep-tracer-cpp/issues/106
+const size_t LightStepDriver::default_min_flush_spans =


Should the concurrency value passed in through the command line be used first then default to the hardware concurrency if not specified?

envoy/source/server/options_impl.cc

Line 53 in c92a301

TCLAP::ValueArg<uint32_t> concurrency("", "concurrency", "# of worker threads to run", false,

@rnburn I gave this more thought and now believe we should just fill in a constant here. Envoy's architecture is that every CPU is an independent worker, so as long as the amount of memory available scales with the number of processors, there's no reason to limit the buffering of LightStep spans according to processor count.

Will you adjust this to use a fixed constant of 200? That avoids the question Constance asks here.

Update to use 200

jmacd · 2018-08-01T17:59:30Z

source/extensions/tracers/lightstep/lightstep_tracer_impl.cc

-        [this] { return runtime_.snapshot().getInteger("tracing.lightstep.min_flush_spans", 5U); }};
+
+    tls_options.max_buffered_spans = std::function<size_t()>{[this] {
+      return runtime_.snapshot().getInteger("tracing.lightstep.min_flush_spans",


We (LightStep) are currently experiencing issues resulting from excessively small Envoy reports. Envoy will consume more memory buffering requests, but have less overhead sending small RPCs to LightStep satellites.

There isn't an ideal setting here, but the current default is an order of magnitude or two out of line with what we'd like.

jmacd · 2018-08-01T18:01:35Z

source/extensions/tracers/lightstep/lightstep_tracer_impl.cc

+// creates separate tracers for each thread.
+//
+// See https://github.com/lightstep/lightstep-tracer-cpp/issues/106
+const size_t LightStepDriver::default_min_flush_spans =


@rnburn I gave this more thought and now believe we should just fill in a constant here. Envoy's architecture is that every CPU is an independent worker, so as long as the amount of memory available scales with the number of processors, there's no reason to limit the buffering of LightStep spans according to processor count.

Will you adjust this to use a fixed constant of 200? That avoids the question Constance asks here.

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

ccaraman

+1 one small nit

ccaraman · 2018-08-02T20:48:43Z

source/extensions/tracers/lightstep/lightstep_tracer_impl.h

  Runtime::Loader& runtime() { return runtime_; }
  LightstepTracerStats& tracerStats() { return tracer_stats_; }

+  static const size_t default_min_flush_spans;


nit: static const vars are all upper case

ccaraman · 2018-08-02T20:50:39Z

@htuch final pass please - LGTM - just one nit

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

htuch

Looks good, two nits.

htuch · 2018-08-03T14:41:49Z

source/extensions/tracers/lightstep/lightstep_tracer_impl.h

  Runtime::Loader& runtime() { return runtime_; }
  LightstepTracerStats& tracerStats() { return tracer_stats_; }

+  static const size_t DEFAULT_MIN_FLUSH_SPANS;


Nit: Prefer DefaultMinFlushSpans for Envoy recommended constant style.

htuch · 2018-08-03T14:42:21Z

source/extensions/tracers/lightstep/lightstep_tracer_impl.cc

    : driver_(driver) {}

+// If the default min_flush_spans value is too small, the larger number of reports can overwhelm
+// LightStep's sattelites. Hence, we need to choose a number that's large enough; though, it's


Nit: typo sattelites

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn added 2 commits July 27, 2018 19:14

Use larger default for min_flush_spans.

55dbef1

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Document default.

b598198

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn added 4 commits July 30, 2018 11:40

Fix test cases.

ecf6bbc

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Change include for thread.

6578a2a

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Fix initialization of default_min_spans.

10c1565

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Fix name error.

742f1d6

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

mattklein123 assigned ccaraman Jul 31, 2018

ccaraman reviewed Aug 1, 2018

View reviewed changes

jmacd suggested changes Aug 1, 2018

View reviewed changes

Use fixed value for default_min_spans.

9837d5a

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

ccaraman previously approved these changes Aug 2, 2018

View reviewed changes

ccaraman assigned htuch Aug 2, 2018

Style fix.

d235aff

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn dismissed ccaraman’s stale review via d235aff August 2, 2018 21:24

mattklein123 previously approved these changes Aug 2, 2018

View reviewed changes

Remove unused header.

cd98a45

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn dismissed mattklein123’s stale review via cd98a45 August 2, 2018 23:20

htuch suggested changes Aug 3, 2018

View reviewed changes

Style and typo fixes.

dffc572

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

mattklein123 approved these changes Aug 3, 2018

View reviewed changes

htuch approved these changes Aug 5, 2018

View reviewed changes

htuch merged commit 7b03f2e into envoyproxy:master Aug 5, 2018

Conversation

rnburn commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnburn commented Jul 30, 2018

Uh oh!

ccaraman commented Jul 30, 2018

Uh oh!

rnburn commented Jul 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccaraman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccaraman commented Aug 2, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rnburn commented Jul 30, 2018 •

edited

Loading