tracing: Add trace sampling configuration to the route, to override the route level #6986

objectiser · 2019-05-17T17:59:04Z

Description: Enable trace sampling to be overridden at the route level.

Risk Level: low
Testing: added unit tests and manually updated examples/jaeger-tracing to try it out.
Docs Changes: Assume model docs are autogenerated. Not sure if any other docs need to be changed.
Release Notes:
Sampling associated with individual routes can now be overridden to define route specific values.

Fixes #6915

dio · 2019-05-18T02:58:53Z

/retest

repokitteh-read-only · 2019-05-18T02:58:57Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6986 (comment) was created by @dio.

see: more, trace.

objectiser · 2019-05-18T22:25:54Z

@dio Coverage now fixed.

objectiser · 2019-05-22T13:12:52Z

@dio @mattklein123 Is this ok to merge?

mattklein123 · 2019-05-22T14:32:24Z

@dio can you take a first pass?

dio

Looks good overall, I can see this is following the pattern of the decorator. Just one question and do you mind to add the release note to the version_history as an update?

source/common/http/conn_manager_impl.cc

objectiser · 2019-05-22T21:55:43Z

@dio Added version_history entry.

mattklein123 · 2019-05-23T02:18:36Z

@objectiser can you fix format and I will take a look?

/wait

mattklein123

Thanks generally looks great. Flushing out a few comments.

/wait

mattklein123 · 2019-05-23T15:39:06Z

api/envoy/api/v2/route/route.proto

+  // 'tracing.client_sampling' in the :ref:`HTTP Connection Manager
+  // <config_http_conn_man_runtime>`.
+  // Default: 100%
+  envoy.type.Percent client_sampling = 1;


Since we have an opportunity here to "redo" the API can we use FractionalPercent? It performs better and also allows for smaller percentages. Also, I think having the defaults be 100% was a mistake in the original tracing code. I guess we should be consistent, but do you think we should consider changing them to 0 potentially? WDYT?

+1 to FractionalPercent.

Regarding changing the sampling - this was done in Istio a while ago, from 100% to 1%, but users still raise issues regularly about not being able to see tracing data - and have to be directed to the FAQ on setting the sampling rate. So there are pros and cons with any setting - but possibly 0 is good for envoy as it essentially means tracing is disabled by default and has to be explicitly configured with a suitable sampling rate.

mattklein123 · 2019-05-23T15:39:29Z

include/envoy/router/router.h

+   * This method returns the client sampling percentage.
+   * @return the client sampling percentage
+   */
+  virtual uint64_t getClientSampling() const PURE;


Per above these should return fractional percent objects.

source/common/http/async_client_impl.h

mattklein123 · 2019-05-23T15:41:21Z

source/common/http/conn_manager_impl.cc

  }
  ASSERT(stream_info_.downstreamRemoteAddress() != nullptr);

  ASSERT(!cached_route_);
  refreshCachedRoute();
+
+  if (!state_.is_internally_created_) {


Sorry why is this new check necessary? Can you add a comment?

Sure can add a comment - essentially previously the call to mutateTracingRequestHeader was performed inside mutateRequestHeaders which was subject to the same condition just above. So thought, even though the call has been pulled out from mutateRequestHeaders, it should still be subject to the same condition? Let me know if not correct.

Is this condition only true for the initial processing of a request at the first proxy? In which case this would prevent the request id being modified mid way through a mesh I assume.

mattklein123 · 2019-05-23T15:42:45Z

source/common/router/config_impl.h

@@ -650,6 +677,7 @@ class RouteEntryImplBase : public RouteEntry,
  const std::multimap<std::string, std::string> opaque_config_;

  const DecoratorConstPtr decorator_;
+  const RouteTracingConstPtr routeTracing_;


nit: route_tracing_

…he settings defined on the filter Signed-off-by: Gary Brown <gary@brownuk.com>

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123

Thanks, looks great. A few small comments.

/wait

mattklein123 · 2019-05-26T14:00:07Z

source/common/http/conn_manager_utility.cc

@@ -234,23 +234,30 @@ void ConnectionManagerUtility::mutateTracingRequestHeader(HeaderMap& request_hea
    return;
  }

+  envoy::type::FractionalPercent client_sampling = config.tracingConfig()->client_sampling_;


can we use a const pointer for these variables to avoid the copy?

source/common/http/conn_manager_config.h

mattklein123 · 2019-05-26T14:06:09Z

source/common/router/config_impl.cc

@@ -324,6 +324,22 @@ void DecoratorImpl::apply(Tracing::Span& span) const {

 const std::string& DecoratorImpl::getOperation() const { return operation_; }

+RouteTracingImpl::RouteTracingImpl(const envoy::api::v2::route::Tracing& tracing)


I don't think you are defaulting these to 100%? Can you double check this and add tests if that is what we want to do for defaults? (Or change the docs).

Based on your previous comment, I was wondering whether a default of 0% would be appropriate here, as they are specific to the route - however the overall_sampling default of 0% does not seem right.

So think it may be better to use 100% for consistency now, and then if the decision is made to change to 0% at a later date, that can be done at both levels.

… change route tracing sampling defaults to 100% Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123

Thanks, looks great. 2 small comments. @dio any further comments?

/wait

mattklein123 · 2019-05-27T15:47:56Z

source/common/router/config_impl.cc

+  }
+  if (!tracing.has_random_sampling()) {
+    random_sampling_.set_numerator(10000);
+    random_sampling_.set_denominator(envoy::type::FractionalPercent::TEN_THOUSAND);


nit: I don't think there is any reason to actually set the denominator here to 10,000, right? I think just making it 100/100 like the others is sufficient? Mostly just trying to avoid questions later about why this is 10,000 here.

mattklein123 · 2019-05-27T15:49:25Z

source/extensions/filters/network/http_connection_manager/config.cc

+    client_sampling.set_numerator(
+        tracing_config.has_client_sampling() ? tracing_config.client_sampling().value() : 100);
+    envoy::type::FractionalPercent random_sampling;
+    random_sampling.set_numerator(


nit: Can you add a small TODO/comment here that random sampling historically was an integer and default to out of 10,000 but we should deprecate that and move to a straight fractional percent config? The way we have this now is pretty confusing for historical reasons.

dio · 2019-05-27T16:09:27Z

This is nice. Thanks! @mattklein123 no more comments from me.

dio

Modulo Matt's.

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2019-05-27T21:47:01Z

@mattklein123 Done

mattklein123

Thank you!

/retest

repokitteh-read-only · 2019-05-28T03:10:18Z

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #6986 (review) was submitted by @mattklein123.

see: more, trace.

dio · 2019-05-28T03:13:44Z

/azp run

objectiser force-pushed the routetraceconfig branch 2 times, most recently from 4cb46dd to 2f4e82b Compare May 17, 2019 18:02

mattklein123 assigned dio May 17, 2019

mattklein123 self-assigned this May 22, 2019

dio changed the title ~~tracing: Add trace sampling configuration to the route, to override t…~~ tracing: Add trace sampling configuration to the route, to override the route level May 22, 2019

dio suggested changes May 22, 2019

View reviewed changes

source/common/http/conn_manager_impl.cc Show resolved Hide resolved

objectiser force-pushed the routetraceconfig branch from 1be06e8 to 1ca93dd Compare May 22, 2019 21:54

repokitteh-read-only bot added waiting and removed waiting labels May 23, 2019

mattklein123 requested changes May 23, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 23, 2019

objectiser added 7 commits May 25, 2019 23:08

tracing: Add trace sampling configuration to the route, to override t…

a79bd03

…he settings defined on the filter Signed-off-by: Gary Brown <gary@brownuk.com>

Fix format issues

31e4785

Signed-off-by: Gary Brown <gary@brownuk.com>

Add test check for route tracing config

8adc41d

Signed-off-by: Gary Brown <gary@brownuk.com>

Fix test

917a845

Signed-off-by: Gary Brown <gary@brownuk.com>

Include details in the version_history

af128ab

Signed-off-by: Gary Brown <gary@brownuk.com>

Fix format

c89a92f

Signed-off-by: Gary Brown <gary@brownuk.com>

Change to use FractionalPercent

69a0441

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the routetraceconfig branch from d3ccb69 to 69a0441 Compare May 25, 2019 23:02

repokitteh-read-only bot removed the waiting label May 25, 2019

mattklein123 requested changes May 26, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 26, 2019

Use const ptr to avoid copy, add test for default sampling values and…

2e19997

… change route tracing sampling defaults to 100% Signed-off-by: Gary Brown <gary@brownuk.com>

repokitteh-read-only bot removed the waiting label May 26, 2019

mattklein123 requested changes May 27, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label May 27, 2019

dio previously approved these changes May 27, 2019

View reviewed changes

Change random_sampling default to 100/100, and add TODO comment

9d582c3

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser dismissed dio’s stale review via 9d582c3 May 27, 2019 18:17

repokitteh-read-only bot removed the waiting label May 27, 2019

mattklein123 approved these changes May 28, 2019

View reviewed changes

mattklein123 merged commit e353807 into envoyproxy:master May 28, 2019

objectiser deleted the routetraceconfig branch May 28, 2019 07:07

mitchdraft mentioned this pull request Jul 24, 2019

tracing: route plugin k8sgateway/k8sgateway#880

Closed

This was referenced Sep 10, 2019

Not able to see any traces from my req in jaeger istio/istio#16985

Closed

Fix random sample fraction percent #8205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: Add trace sampling configuration to the route, to override the route level #6986

tracing: Add trace sampling configuration to the route, to override the route level #6986

objectiser commented May 17, 2019 •

edited by dio

Loading

dio commented May 18, 2019

repokitteh-read-only bot commented May 18, 2019

objectiser commented May 18, 2019

objectiser commented May 22, 2019

mattklein123 commented May 22, 2019

dio left a comment

objectiser commented May 22, 2019

mattklein123 commented May 23, 2019

mattklein123 left a comment

mattklein123 May 23, 2019

objectiser May 23, 2019

mattklein123 May 23, 2019

mattklein123 May 23, 2019

objectiser May 23, 2019

mattklein123 May 23, 2019

mattklein123 left a comment

mattklein123 May 26, 2019

mattklein123 May 26, 2019

objectiser May 26, 2019

mattklein123 left a comment

mattklein123 May 27, 2019

mattklein123 May 27, 2019

dio commented May 27, 2019

dio left a comment

objectiser commented May 27, 2019

mattklein123 left a comment

repokitteh-read-only bot commented May 28, 2019

dio commented May 28, 2019

		@@ -324,6 +324,22 @@ void DecoratorImpl::apply(Tracing::Span& span) const {

		const std::string& DecoratorImpl::getOperation() const { return operation_; }

		RouteTracingImpl::RouteTracingImpl(const envoy::api::v2::route::Tracing& tracing)

tracing: Add trace sampling configuration to the route, to override the route level #6986

tracing: Add trace sampling configuration to the route, to override the route level #6986

Conversation

objectiser commented May 17, 2019 • edited by dio Loading

dio commented May 18, 2019

repokitteh-read-only bot commented May 18, 2019

objectiser commented May 18, 2019

objectiser commented May 22, 2019

mattklein123 commented May 22, 2019

dio left a comment

Choose a reason for hiding this comment

objectiser commented May 22, 2019

mattklein123 commented May 23, 2019

mattklein123 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 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

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented May 27, 2019

dio left a comment

Choose a reason for hiding this comment

objectiser commented May 27, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented May 28, 2019

dio commented May 28, 2019

objectiser commented May 17, 2019 •

edited by dio

Loading