request_id_utils: add new extension system by euroelessar · Pull Request #10429 · envoyproxy/envoy

euroelessar · 2020-03-18T05:18:54Z

Description: Allows for extensions to customize the behavior of Request ID manipulation. This replaces the UUIDUtils class and makes it generic. This makes it possible to customize how request IDs are used for tracing and logging.

Fixes #7624

Risk Level: Medium/High. Default behavior is kept the same but UUIDUtils are abstracted away.
Testing: Add unit tests
Docs Changes: Updated
Release Notes:

…in issue envoyproxy#7624. Signed-off-by: Ross Delinger <rossd@dropbox.com>

Signed-off-by: Ross Delinger <rossd@dropbox.com>

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only · 2020-03-18T05:19:01Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10429 was opened by euroelessar.

see: more, trace.

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

mattklein123

At a high level this looks good. Flushing out some comments from a first pass. Thank you!

/wait

mattklein123 · 2020-03-18T21:50:41Z

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

+//
+// 3. Tracing decision (sampled, forced, etc) is set in 14th byte of the UUID.
+
+message UUIDConfig {


I don't think we need a config for this. Can we just have this be the default class/impl that is loaded if there is no typed_config specified?

Any recommendation where should I put a documentation about the default behavior in that case? That and increasing amount of shared logic (between default and custom implementations) were main motivations for introduction of this proto package.

Maybe just move the documentation above the typed_config field to explain what the behavior is if it's not set?

include/envoy/request_id_utils/request_id_utils.h

source/common/access_log/access_log_impl.cc

mattklein123 · 2020-03-18T22:04:36Z

source/common/http/conn_manager_impl.cc

         "Either routeConfigProvider or scopedRouteConfigProvider should be set in "
         "ConnectionManagerImpl.");
+
+  stream_info_.setRequestIDUtils(connection_manager.config_.requestIDUtils());


Why does stream info need the request ID utiltiies? I'm wondering if whatever stat it is being used for can be stored instead?

Looks like it's used in two cases:

AccessLog::TraceableRequestFilter and ConnectionManagerImpl::ActiveStream to determine if request is traced (using Tracing::HttpTracerUtility)

AccessLog::RuntimeFilter uses it for making deterministic logging sampling decision

Given that AccessLog instances are shared across all L4/L5 connection managers, and RequestIDUtils is configured on per-manager basis we need a side channel for passing this information.

I'm worried about preserving derivatives (boolean isTraced and raw sampling number) as they can become out of sync if request id is changed mid-flight. It doesn't look like any existing extensions currently do it though.

OK fair enough I think that's fine.

source/common/http/conn_manager_utility.cc

source/common/request_id_utils/request_id_utils_impl.cc

source/extensions/extensions_build_config.bzl

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-03-18T23:44:47Z

source/extensions/request_id_utils/uuid/uuid_impl.cc

+
+void UUIDUtils::maybePreserveRequestIDInResponse(Http::ResponseHeaderMap& response_headers,
+                                                 const Http::RequestHeaderMap& request_headers) {
+  if (request_headers.EnvoyForceTrace() && request_headers.RequestId()) {


Alternatively check request_headers.EnvoyForceTrace() can be kept outside of this implementation. In that case we add a configuration option in separate pull request to allow always sending request ID header back even without forced tracing. This way we will be able omit "maybe" part of the method and keep if logic outside.

It's generally useful for correlating requests with client-side logs even if tracing itself is disabled for this particular request.

Agreed that makes sense to me and has been a requested feature before.

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

htuch · 2020-03-19T00:36:26Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // generation, validation, and associated tracing operations.
+  //
+  // If not set, Envoy uses the :ref:`default UUID-based behavior <extension_envoy.request_id_utils.uuid>`.
+  RequestIDUtils request_id_utils = 36;


Playing naming poker, I think something like RequestIdExtension is probably a better message/field name. The "utils" concept is strictly less descriptive.

Sure, can rename it. What's about relevant classes in code, should I keep them as is?

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-03-26T05:57:43Z

Something is wrong with circleci, why did it take 6 hours to run tests?..

mattklein123

Thanks this is super awesome. Just some small comments and I think we can ship!

/wait

mattklein123 · 2020-03-26T17:58:25Z

include/envoy/http/request_id_extension.h

+  virtual void set(Http::RequestHeaderMap& request_headers) PURE;
+
+  /**
+   * Ensure that a request is configured with a request id. Do not override an ID if request has one


+1 let's merge set/ensure and just have a boolean param? I think that would be simpler to understand.

mattklein123 · 2020-03-26T17:59:36Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // 2. Request ID is a universally unique identifier (UUID).
+  //
+  // 3. Tracing decision (sampled, forced, etc) is set in 14th byte of the UUID.
+  RequestIDExtension request_id_extension = 36;


Can you add a release note about this? Potentially also update https://www.envoyproxy.io/docs/envoy/latest/extending/extending as well as maybe mention this extensibility somewhere in the HTTP/tracing arch docs?

mattklein123 · 2020-03-26T18:02:11Z

source/common/http/request_id_extension_uuid_impl.h

+namespace Envoy {
+namespace Http {
+
+class UUIDRequestIDExtension : public RequestIDExtension {


nit: small comment above that this is the default implementation if none is configured?

mattklein123 · 2020-03-26T18:02:56Z

source/common/tracing/http_tracer_impl.cc


-  if (!request_headers.RequestId()) {
+  Http::RequestIDExtensionSharedPtr rid_extension = stream_info.getRequestIDExtension();
+  if (rid_extension == nullptr) {


This can't happen, right? Or can it happen earlier in server load? If so maybe comment? It seems like maybe this should actually be a function call into the extension to check for something and this might be a bug?

mattklein123 · 2020-03-26T18:03:45Z

source/extensions/extensions_build_config.bzl


    "envoy.filters.udp_listener.udp_proxy":             "//source/extensions/filters/udp/udp_proxy:config",
-
+    


nit: revert since no file changes.

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

mattklein123 · 2020-03-27T18:55:27Z

Thanks awesome work. Can you merge master again which should fix clang-tidy (and also fix any issues that show up as a result of that).

/wait

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar · 2020-03-27T20:27:08Z

It looks like clang-tidy has either false-positive or unrelated errors, e.g. it complains about using throw at envoy/buffer/buffer.h (not touched by this pr), or about using virtual in virtual ~RequestIDExtensionFactory() = default; definition.

mattklein123 · 2020-03-27T20:51:57Z

The throw issue should be fixed. Did you fully merge orgin/master? cc @lizan

euroelessar · 2020-03-27T21:02:31Z

I've merged 913afb0, 4e16a37 is part of it

mattklein123 · 2020-03-27T21:06:05Z

OK I guess there is another issue here. cc @lizan

We can force merge if there is no real issue to fix.

mattklein123

Thank you!!

Ross Delinger and others added 13 commits February 23, 2020 19:37

Introduce RequestIDUtils extensions to override behavior as outlined …

f469ba4

…in issue envoyproxy#7624. Signed-off-by: Ross Delinger <rossd@dropbox.com>

fix last few compilation errors

868195f

Signed-off-by: Ross Delinger <rossd@dropbox.com>

fix formatting

a0b75c0

Signed-off-by: Ross Delinger <rossd@dropbox.com>

fix stream_info request id utils mocks

12cc6a8

Signed-off-by: Ross Delinger <rossd@dropbox.com>

initial code review fixes

0459391

Signed-off-by: Ross Delinger <rossd@dropbox.com>

swap request_id_utils config to TypedConfig

f65a1c7

Signed-off-by: Ross Delinger <rossd@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into request-id-…

183c43e

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

delete unused fields

bd4a6f7

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

Merge branch 'master' of github.com:envoyproxy/envoy into request-id-…

6d6bbf8

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

shadow

ffc0dcc

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

use typed_url + docs

c022ce6

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

docs

b29f8e7

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

use typed config

e66c17a

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

euroelessar requested review from alyssawilk and mattklein123 as code owners March 18, 2020 05:18

repokitteh-read-only bot added the api label Mar 18, 2020

euroelessar mentioned this pull request Mar 18, 2020

implement new request_id_utils extension system #10018

Closed

mattklein123 self-assigned this Mar 18, 2020

Ruslan Nigmatullin added 2 commits March 18, 2020 11:39

Merge branch 'master' of github.com:envoyproxy/envoy into request-id-…

a724f1f

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

rm FIXME and make setRequestID doc more explicit

4dc49fd

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

mattklein123 requested changes Mar 18, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 18, 2020

Ruslan Nigmatullin added 2 commits March 18, 2020 16:10

address TODO for preserving request ID in response

1d4f848

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

CR

3686b98

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only bot removed the waiting label Mar 18, 2020

euroelessar commented Mar 18, 2020

View reviewed changes

fix fmt

b374a13

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

htuch reviewed Mar 19, 2020

View reviewed changes

rm proto config and rm maybe part

f8e1d4a

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

fix access log test

c6bd097

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only bot removed the waiting label Mar 25, 2020

mattklein123 requested changes Mar 26, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 26, 2020

Ruslan Nigmatullin added 4 commits March 26, 2020 17:26

Merge branch 'master' of github.com:envoyproxy/envoy into request-id-…

a48f1d2

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

merge set and ensure

4182509

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

docs

322e607

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

fmt

aee66bd

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only bot removed the waiting label Mar 27, 2020

Ruslan Nigmatullin added 3 commits March 26, 2020 19:42

remove nullptr check

09b683c

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

regen proto

20d40a8

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

use override

4b5b751

Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only bot added the waiting label Mar 27, 2020

Merge branch 'master' of github.com:envoyproxy/envoy into request-id-…

0962049

…utils Signed-off-by: Ruslan Nigmatullin <elessar@dropbox.com>

repokitteh-read-only bot removed the waiting label Mar 27, 2020

mattklein123 added the waiting label Mar 27, 2020

mattklein123 approved these changes Mar 27, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Mar 27, 2020

mattklein123 merged commit 56537f6 into envoyproxy:master Mar 27, 2020

euroelessar mentioned this pull request Apr 16, 2020

request_id: Add option to always set request id in response #10808

Merged

rgs1 mentioned this pull request Jan 11, 2021

stream info: cleanup address handling #14432

Merged


		"envoy.filters.udp_listener.udp_proxy": "//source/extensions/filters/udp/udp_proxy:config",

Conversation

euroelessar commented Mar 18, 2020 • edited by mattklein123 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Mar 18, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

euroelessar commented Mar 26, 2020

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!

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 commented Mar 27, 2020

Uh oh!

euroelessar commented Mar 27, 2020

Uh oh!

mattklein123 commented Mar 27, 2020

Uh oh!

euroelessar commented Mar 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Mar 27, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

euroelessar commented Mar 18, 2020 •

edited by mattklein123

Loading

euroelessar commented Mar 27, 2020 •

edited

Loading