tracing: add option to choice mutation policy for RequestID extension by Shikugawa · Pull Request #16770 · envoyproxy/envoy

Shikugawa · 2021-06-02T11:47:10Z

Signed-off-by: Shikugawa rei@tetrate.io

Commit Message: Close #16530. In the previous implementation, the sampling decision was determined by the UUID and additional headers. In this state, without the additional headers, the sampling decision depends entirely on the UUID. In this case, in an environment where Envoy is mixed with other sidecars that have different request ID generation procedures, Envoy receiving requests from other sidecars will always send a span with invalid sampling. This behavior is considered to be unnatural as the default behavior. Therefore, in this PR, we provide the mutation_policy option for Request ID extension. When this option is configured as SKIP, it will always skip the request ID mutation and execute request ID (currently UUID) independent sampling decision algorithm and enable sampling of the span by default. With this flag in place, the sampling is completely determined by the additional header, which makes the behavior intuitive and independent of the sidecar type.
Additional Description:
Risk Level: Low
Testing: Unit
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

cc @basvanbeek

Signed-off-by: Shikugawa <rei@tetrate.io>

repokitteh-read-only · 2021-06-02T11:47:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16770 was opened by Shikugawa.

see: more, trace.

adisuissa

Thanks for working on this!
Added an API comment.

Also would suggest adding more info to the doc.

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

source/common/http/conn_manager_utility.cc

…ate-sampling Signed-off-by: Shikugawa <rei@tetrate.io>

Signed-off-by: Shikugawa <rei@tetrate.io>

adisuissa

Can you please also update docs/root/intro/arch_overview/observability/tracing.rst?

adisuissa · 2021-06-09T01:54:44Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

+    // Trace reason is decided by request_id by default. If true, these sampling strategy decisions
+    // will be decided by only tracing provider and :ref:`x-request-id<config_http_conn_man_headers_x-request-id>`
+    // never be modified.


Not sure if I understand this correctly, but please let me know if I'm wrong here:

Suggested change

// Trace reason is decided by request_id by default. If true, these sampling strategy decisions

// will be decided by only tracing provider and :ref:`x-request-id<config_http_conn_man_headers_x-request-id>`

// never be modified.

// By default, tracing requires the :ref:`x-request-id <config_http_conn_man_headers_x-request-id>` header to be set, and modifies its value.

// If true, the request will be traced, and *x-request-id* will not be modified.

I think that we should add that the request will bypass x-request-id based tracing decision.

Should this be an enum? Do we want to maybe support other modes of operation in the future?

What operation will be expected in the future?

I was interested in your perspective on this.

@htuch It can be changed as follows.

enum TraceRequestIdSampleDecision { None, Bypass, // It acts as `bypass_sampling_with_request_id` is true. }

WDYT? cc. @lizan

Sure, with NONE, BYPASS. If you have high confidence we would never have some other trace sample strategy with request ID I'm fine with the current bool, just would like to make sure this is deliberative.

Signed-off-by: Shikugawa <rei@tetrate.io>

Shikugawa · 2021-06-17T11:10:19Z

@htuch @mattklein123 I'm considering that we might add a new tracing stat forced_sampling that means not sampled randomly and new tracing reason Tracing::Reason::SamplingForced. WDYT?

htuch · 2021-06-18T02:09:27Z

There are already a bunch of different forced Tracing::Reason, e.g. ServiceForced, ClientForced; can you make it clearer what this is in the name?

htuch

/lgtm api

htuch · 2021-06-18T02:13:07Z

Over to @mattklein123 for tracing implementation review.

alyssawilk · 2021-06-22T13:31:46Z

@mattklein123 is out for the week. I can do a pass while he's out but looking this over I agree with Adi this should include an update to docs/root/intro/arch_overview/observability/tracing.rst as I'm not quite sure from the API documentation what this option provides. Mind adding that for now and also seeing if the coverage failure is real?

/wait

Signed-off-by: Shikugawa <rei@tetrate.io>

mattklein123

Thanks for working on this. I fixed a similar issue recently with users requesting to not pack trace status into the UUID. See pack_trace_reason here: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/request_id/uuid/v3/uuid.proto

IMO it would be better to add a new configuration option for the default request ID provider to not use request ID for sampling. I think this would be a bit cleaner as it would be colocated with the other related option and also avoid adding new configuration to HCM. WDYT?

/wait

Shikugawa · 2021-06-29T02:59:37Z

IMO it would be better to add a new configuration option for the default request ID provider to not use request ID for sampling. I think this would be a bit cleaner as it would be colocated with the other related option and also avoid adding new configuration to HCM. WDYT?

SGTM. Let me change.

mattklein123

Thanks for iterating. Starting with an API comment.

/wait

api/envoy/extensions/request_id/uuid/v3/uuid.proto

Signed-off-by: Shikugawa <rei@tetrate.io>

mattklein123

Thanks LGTM with small comments.

/wait

docs/root/intro/arch_overview/observability/tracing.rst

Signed-off-by: Shikugawa <rei@tetrate.io>

lizan

defer to @mattklein123

mattklein123

/wait

api/envoy/extensions/request_id/uuid/v3/uuid.proto

docs/root/intro/arch_overview/observability/tracing.rst

Signed-off-by: Shikugawa <rei@tetrate.io>

mattklein123

/wait

mattklein123 · 2021-07-07T14:52:33Z

api/envoy/extensions/request_id/uuid/v3/uuid.proto

-  // Set whether to use :ref:`x-request-id<config_http_conn_man_headers_x-request-id>` or not.
-  // This defaults to true.
+  // Set whether to use :ref:`x-request-id<config_http_conn_man_headers_x-request-id>` for sampling or not.
+  // This defaults to true. See the :ref:`context propagation <arch_overview_tracing_context_propagation>`


Suggested change

// This defaults to true. See the :ref:`context propagation <arch_overview_tracing_context_propagation>`

// This defaults to true. See the :ref:`context propagation <arch_overview_tracing_context_propagation>` overview

Signed-off-by: Shikugawa <rei@tetrate.io>

mattklein123

Thanks!

…envoyproxy#16770) Signed-off-by: Shikugawa <rei@tetrate.io>

tracing: delegate sampling decision logic to tracing provider

e85fb65

Signed-off-by: Shikugawa <rei@tetrate.io>

Shikugawa requested review from alyssawilk and mattklein123 as code owners June 2, 2021 11:47

repokitteh-read-only bot added the api label Jun 2, 2021

repokitteh-read-only bot assigned adisuissa Jun 2, 2021

junr03 assigned htuch Jun 2, 2021

adisuissa reviewed Jun 3, 2021

View reviewed changes

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto Outdated Show resolved Hide resolved

source/common/http/conn_manager_utility.cc Outdated Show resolved Hide resolved

alyssawilk added the waiting label Jun 3, 2021

Shikugawa added 2 commits June 8, 2021 21:21

Merge branch 'main' of https://github.com/envoyproxy/envoy into deleg…

e3390e7

…ate-sampling Signed-off-by: Shikugawa <rei@tetrate.io>

fix

2242a5f

Signed-off-by: Shikugawa <rei@tetrate.io>

repokitteh-read-only bot removed the waiting label Jun 8, 2021

Shikugawa changed the title ~~tracing: delegate sampling decision logic to tracing provider~~ tracing: add option to bypass sampling decision by x-request-id Jun 8, 2021

adisuissa reviewed Jun 9, 2021

View reviewed changes

fix

adf26d7

Signed-off-by: Shikugawa <rei@tetrate.io>

Shikugawa force-pushed the delegate-sampling branch from ea8eba5 to adf26d7 Compare June 17, 2021 07:37

htuch reviewed Jun 18, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jun 18, 2021

htuch assigned mattklein123 Jun 18, 2021

repokitteh-read-only bot added the waiting label Jun 22, 2021

coverage failure

9fab5d1

Signed-off-by: Shikugawa <rei@tetrate.io>

repokitteh-read-only bot removed the waiting label Jun 23, 2021

add docs

a5c2a74

Signed-off-by: Shikugawa <rei@tetrate.io>

mattklein123 requested changes Jun 28, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 28, 2021

repokitteh-read-only bot added api and removed waiting labels Jun 29, 2021

Shikugawa changed the title ~~tracing: add option to bypass sampling decision by x-request-id~~ tracing: add option to choice mutation policy for RequestID extension Jun 29, 2021

mattklein123 requested changes Jun 29, 2021

View reviewed changes

api/envoy/extensions/request_id/uuid/v3/uuid.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jun 29, 2021

htuch removed their assignment Jul 1, 2021

revert

05dae2d

Signed-off-by: Shikugawa <rei@tetrate.io>

repokitteh-read-only bot removed the waiting label Jul 2, 2021

mattklein123 requested changes Jul 2, 2021

View reviewed changes

docs/root/intro/arch_overview/observability/tracing.rst Outdated Show resolved Hide resolved

docs/root/intro/arch_overview/observability/tracing.rst Outdated Show resolved Hide resolved

docs/root/intro/arch_overview/observability/tracing.rst Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jul 2, 2021

polish doc

e71d94f

Signed-off-by: Shikugawa <rei@tetrate.io>

repokitteh-read-only bot removed the waiting label Jul 2, 2021

lizan previously approved these changes Jul 2, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 2, 2021

mattklein123 requested changes Jul 6, 2021

View reviewed changes

api/envoy/extensions/request_id/uuid/v3/uuid.proto Outdated Show resolved Hide resolved

docs/root/intro/arch_overview/observability/tracing.rst Outdated Show resolved Hide resolved

docs/root/intro/arch_overview/observability/tracing.rst Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jul 6, 2021

doc fix

bdb1124

Signed-off-by: Shikugawa <rei@tetrate.io>

Shikugawa dismissed lizan’s stale review via bdb1124 July 7, 2021 05:24

repokitteh-read-only bot added api and removed waiting labels Jul 7, 2021

mattklein123 requested changes Jul 7, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 7, 2021

doc fix

3ef5122

Signed-off-by: Shikugawa <rei@tetrate.io>

repokitteh-read-only bot removed the waiting label Jul 8, 2021

mattklein123 approved these changes Jul 8, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 8, 2021

mattklein123 merged commit 9170b3e into envoyproxy:main Jul 8, 2021

Shikugawa deleted the delegate-sampling branch July 9, 2021 01:34

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

tracing: add option to choice mutation policy for RequestID extension (…

f913eab

…envoyproxy#16770) Signed-off-by: Shikugawa <rei@tetrate.io>

nheaps-chwy mentioned this pull request Oct 25, 2021

Bug: Client X-Request-Id being passed without tracer traceIDs cause the request to be un-traceable aws/aws-app-mesh-roadmap#373

Closed

	// This defaults to true. See the :ref:`context propagation <arch_overview_tracing_context_propagation>`
	// This defaults to true. See the :ref:`context propagation <arch_overview_tracing_context_propagation>` overview

Conversation

Shikugawa commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 2, 2021

Uh oh!

adisuissa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adisuissa 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!

Choose a reason for hiding this comment

Uh oh!

Shikugawa Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Shikugawa commented Jun 17, 2021

Uh oh!

htuch commented Jun 18, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Jun 18, 2021

Uh oh!

alyssawilk commented Jun 22, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Shikugawa commented Jun 29, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Shikugawa commented Jun 2, 2021 •

edited

Loading

adisuissa left a comment •

edited

Loading

Shikugawa Jun 14, 2021 •

edited

Loading