api: promote tracing operation to listener level by kyessenov · Pull Request #7723 · envoyproxy/envoy

kyessenov · 2019-07-25T21:01:22Z

Signed-off-by: Kuat Yessenov kuat@google.com
Description:

Promote tracing operation field to listener level. This expands the scope of the field to support two use cases:

Tracing TCP connections: istio can send connection events to create a service communication graph. Network filters can benefit from the common knowledge about the intent of the listener/filter chain (client-side vs server-side).
Using ingress/egress designation for other telemetry. The direction of the traffic is a useful label on metrics, and it is not explicit at the moment, unless depending on tracing configuration in HTTP connection manager or naming convention. Both workarounds are not ideal.

Risk Level: low
Testing: all unit tests continue to pass
Docs Changes:
Release Notes:

kyessenov · 2019-07-25T21:02:31Z

I am not a fan of using the default value 0 to mean INGRESS, but alas, the API needs to stay backwards compatible. I'd like to add a third option to mean undefined/middle proxy, but that requires plumbing through all tracers.

kyessenov · 2019-07-25T21:03:25Z

cc @mandarjog @bianpengyuan

Signed-off-by: Kuat Yessenov <kuat@google.com>

snowp

Seems reasonable to me, just one comment on the V1 JSON handling

snowp · 2019-07-26T01:37:00Z

source/common/config/filter_json.cc

-    envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::Tracing::
-        OperationName_Parse(StringUtil::toUpper(json_tracing->getString("operation_name")),
-                            &operation_name);
+    envoy::api::v2::core::TracingOperation operation_name{};


I don't think we need this change since this is for the deprecated V1 config handling

I renamed the proto enum, so this is necessary for build to complete. It has no effect on the wire, since it's the same enum structurally.

ah yeah that makes sense

snowp

LGTM

lizan · 2019-07-26T02:56:25Z

cc @htuch for UDPA visibility / compatibility, this breaks generated code as well.

kyessenov · 2019-07-26T03:02:33Z

I'm fine putting HTTP thingy where it belonged and duplicating the proto. I just think this is less sustainable long-term.

rshriram

..

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

objectiser · 2019-07-26T13:15:43Z

@kyessenov Just wondering if this is a good opportunity to change the concept from "operation" to what it truly represents, i.e. "direction"? Operation can be misleading, as it can also refer to the "operation" being performed on the service.

mattklein123 · 2019-07-26T16:56:50Z

Waiting for @htuch to take a look from the API perspective.

/wait-any

kyessenov · 2019-07-26T17:42:30Z

@objectiser I'm happy to pick a better name. Tracing direction? Traffic direction? Direction?

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2019-07-26T22:22:14Z

@htuch undid changes to HTTP tracing and added a TODO to align it later.

objectiser · 2019-07-27T11:34:28Z

@kyessenov traffic direction LGTM.

htuch · 2019-07-29T01:51:54Z

OK, so this is now good from an API compat standpoint, but I'm questioning why we need this. Can't you use LDS metadata for direction? I'm wondering how universal the notion of ingress/egress is here vs. something that is Istio specific? Envoy is used in a ton of configurations, including edge and middle proxy, so ingress/egress doesn't really capture these, in particular when there is no local application. How do we deal with these?

kyessenov · 2019-07-29T03:12:21Z

@htuch Ingress/egress is used in all HTTP tracers' configurations, so I think it's fairly common apart from istio as well. The issue with middle proxies is a real one, and I don't have a good answer here apart from adding a new enum value to model it. The question should be how would you model a middle proxy in the existing HTTP tracer config, and we can incorporate the answer into this new config.

htuch · 2019-07-29T13:54:04Z

@kyessenov OK. Can you open an issue for that? Also, needs master merge. Otherwise, LGTM.

mattklein123

Please add a config test for this field on the listener, otherwise LGTM. Thank you!

/wait

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2019-07-29T18:15:21Z

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

🐱

Caused by: #7723 was synchronize by kyessenov.

see: more, trace.

kyessenov · 2019-07-29T18:18:50Z

/retest

repokitteh-read-only · 2019-07-29T18:18:54Z

🔨 rebuilding ci/circleci: docs (failed build)

🐱

Caused by: a #7723 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2019-07-29T18:55:51Z

/retest

repokitteh-read-only · 2019-07-29T18:55:54Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #7723 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2019-07-29T19:51:31Z

/retest

repokitteh-read-only · 2019-07-29T19:51:34Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #7723 (comment) was created by @kyessenov.

see: more, trace.

kyessenov · 2019-07-29T19:52:13Z

/azp run

azure-pipelines · 2019-07-29T19:52:18Z

Commenter does not have sufficient privileges for PR 7723 in repo envoyproxy/envoy

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only · 2019-07-30T21:47:23Z

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

🐱

Caused by: #7723 was synchronize by kyessenov.

see: more, trace.

htuch

Thanks!

Fixed

kyessenov force-pushed the ingress_egress branch from 3814dc4 to fb24859 Compare July 25, 2019 21:07

add ingress/egress listener designator

fb1c733

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov force-pushed the ingress_egress branch from fb24859 to fb1c733 Compare July 25, 2019 21:18

dio assigned snowp Jul 25, 2019

snowp suggested changes Jul 26, 2019

View reviewed changes

snowp previously approved these changes Jul 26, 2019

View reviewed changes

snowp assigned mattklein123 Jul 26, 2019

rshriram reviewed Jul 26, 2019

View reviewed changes

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto Show resolved Hide resolved

mattklein123 assigned htuch Jul 26, 2019

repokitteh-read-only bot added the waiting:any label Jul 26, 2019

repokitteh-read-only bot removed the waiting:any label Jul 26, 2019

mattklein123 added the waiting label Jul 26, 2019

revert to v3 compat

61536f0

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed snowp’s stale review via 61536f0 July 26, 2019 22:21

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

mattklein123 previously requested changes Jul 29, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 29, 2019

merge fix

9bc4490

Signed-off-by: Kuat Yessenov <kuat@google.com>

add a unit test

71ec89a

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only bot removed the waiting label Jul 29, 2019

kyessenov mentioned this pull request Jul 29, 2019

api: model edge and middle proxies in tracing configurations #7751

Closed

rename

1f62a53

Signed-off-by: Kuat Yessenov <kuat@google.com>

htuch approved these changes Jul 30, 2019

View reviewed changes

kyessenov mentioned this pull request Jul 31, 2019

Update generated files envoyproxy/go-control-plane#201

Merged

htuch merged commit ca2af97 into envoyproxy:master Jul 31, 2019

lambdai mentioned this pull request Aug 6, 2019

pilot: add traffic direction to listener istio/istio#16080

Merged

LukeShu mentioned this pull request Feb 19, 2020

Prepare for Envoy 1.13 upgrade; migrate from now-disabled deprecated fields emissary-ingress/emissary#2328

Merged

Conversation

kyessenov commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyessenov commented Jul 25, 2019

Uh oh!

kyessenov commented Jul 25, 2019

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

snowp Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

kyessenov Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

snowp Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

lizan commented Jul 26, 2019

Uh oh!

kyessenov commented Jul 26, 2019

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

objectiser commented Jul 26, 2019

Uh oh!

mattklein123 commented Jul 26, 2019

Uh oh!

kyessenov commented Jul 26, 2019

Uh oh!

kyessenov commented Jul 26, 2019

Uh oh!

objectiser commented Jul 27, 2019

Uh oh!

htuch commented Jul 29, 2019

Uh oh!

kyessenov commented Jul 29, 2019

Uh oh!

htuch commented Jul 29, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jul 29, 2019

Uh oh!

kyessenov commented Jul 29, 2019

Uh oh!

repokitteh-read-only bot commented Jul 29, 2019

Uh oh!

kyessenov commented Jul 29, 2019

Uh oh!

repokitteh-read-only bot commented Jul 29, 2019

Uh oh!

kyessenov commented Jul 29, 2019

Uh oh!

repokitteh-read-only bot commented Jul 29, 2019

Uh oh!

kyessenov commented Jul 29, 2019

Uh oh!

azure-pipelines bot commented Jul 29, 2019

Uh oh!

repokitteh-read-only bot commented Jul 30, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

kyessenov commented Jul 25, 2019 •

edited

Loading