tracing: enhance custom tags source #8279

yittg · 2019-09-18T10:13:30Z

Signed-off-by: Yi Tang ssnailtang@gmail.com

Description:

Support to set custom tag from literal string, environment, request header, and metadata associated with request and route in tracing.
Support to config custom tags at the route level also, which would be merged with the connection manager tracing configuration before applied. The former takes priority if configured with the same tag.

Deprecated:

The request_headers_for_tags field in HTTP connection manager has been deprecated in favor of the custom_tags field.

Risk Level: Medium
Testing: unit test
Docs Changes: proto docs
Fixes #8224

repokitteh-read-only · 2019-09-18T10:50:27Z

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

🐱

Caused by: #8279 was synchronize by yittg.

see: more, trace.

objectiser

From a quick look, it seems like a good start. Couple of general points:

Could you rename the field from additional_tags to custom_tags - so the name of the field and type is consistent.
Sampling config can now also be set at the route level - not necessarily for this PR, but might also be good to consider whether we want these custom tags to also be configurable at that level. If so - we need to decide how the route level custom tags are handled - probably override/merge based on whether common tag names are used.

yittg · 2019-09-18T16:41:53Z

@objectiser thanks.

Could you rename the field from additional_tags to custom_tags - so the name of the field and type is consistent.

Fixed.

Sampling config can now also be set at the route level - not necessarily for this PR, but might also be good to consider whether we want these custom tags to also be configurable at that level. If so - we need to decide how the route level custom tags are handled - probably override/merge based on whether common tag names are used.

I'll have a look.

zuercher · 2019-09-18T16:45:41Z

@snowp can you have a look?

snowp

Thanks this looks reasonable to me.

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

source/common/config/filter_json.cc

source/common/json/config_schemas.cc

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

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

yittg · 2019-09-20T06:43:35Z

ping @snowp , would you have another look please?

snowp

Thanks for iterating on this! Definitely moving in the right direction

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

include/envoy/tracing/http_tracer.h

source/common/http/conn_manager_impl.cc

source/common/tracing/http_tracer_impl.cc

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

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

test/common/http/conn_manager_impl_test.cc

mattklein123

Thanks for working on this. Flushing out some API level comments. Let's nail that first and then we can move on to code review. Also please merge mater to pick up the new formatter tooling.

/wait

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

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

yittg · 2019-09-25T05:40:59Z

thanks @mattklein123 , maybe we should new a PR for the API(protos) change first, then do the implementation?

mattklein123 · 2019-09-25T17:13:11Z

thanks @mattklein123 , maybe we should new a PR for the API(protos) change first, then do the implementation?

I think it's fine to do it in this PR, but let's lock down the API first even if CI is not passing.

/wait

kyessenov · 2019-09-25T20:01:12Z

Should we also expose ability to set tags from dynamic metadata from the node, route, or route entry?

api/envoy/type/tracing.proto

yittg · 2019-09-26T02:26:11Z

@kyessenov @mandarjog I think you are describing the same point, both for some kind of dynamic metadata?

objectiser

Looking good - thanks for all the hard work on this!

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

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

objectiser · 2019-09-27T09:23:51Z

api/envoy/type/tracing.proto

+    // environment variable name to obtain the value to populate the tag value.
+    string name = 1 [(validate.rules).string = {min_bytes: 1}];
+
+    // used to populate the tag value is the environment variable is not found.


"value if the ..."

objectiser · 2019-09-27T09:32:48Z

api/envoy/type/tracing.proto

+  }
+
+  // RouteMetadata type custom tag with namespace, path, and default value.
+  message RouteMetadata {


This seems like an exact copy of the RequestMetadata - would be better to just have message Metadata and use for both request_metadata and route_metadata fields below.

objectiser · 2019-09-27T09:33:15Z

api/envoy/type/tracing.proto

+    string default_value = 4;
+  }
+
+  reserved 2, 3;


Why are these reserved when this is a new proto message?

objectiser · 2019-09-27T09:52:53Z

source/common/tracing/http_tracer_impl.cc

+EnvironmentCustomTag::EnvironmentCustomTag(const std::string& tag, const std::string& name,
+                                           const std::string& default_value)
+    : DynamicCustomTag(tag, default_value), name_(name) {}
+absl::string_view EnvironmentCustomTag::obtainValue(const CustomTagContext&) const {


nit: line above

objectiser · 2019-09-27T09:55:29Z

source/common/tracing/http_tracer_impl.cc

+                                           const std::string& default_value)
+    : DynamicCustomTag(tag, default_value), name_(name) {}
+absl::string_view EnvironmentCustomTag::obtainValue(const CustomTagContext&) const {
+  return std::getenv(name_.data());


Might want to only call this on initialisation, rather than per request - to avoid unnecessary system call. This was raised as a point on the istio P&T call when discussing this feature - to ensure it is as efficient as possible.

mattklein123

Thanks, looking good. Flushing out some high level API/interface comments.

/wait

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

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

mattklein123 · 2019-09-27T17:04:53Z

api/envoy/type/tracing.proto

+    // environment variable name to obtain the value to populate the tag value.
+    string name = 1 [(validate.rules).string = {min_bytes: 1}];
+
+    // used to populate the tag value if the environment variable is not found.


Should this be required? If not, what is the behavior if this is empty? Does the tag get populated with an empty value? Not populated? Please document.

mattklein123 · 2019-09-27T17:05:09Z

api/envoy/type/tracing.proto

+    // request header name to obtain the value to populate the tag value.
+    string name = 1 [(validate.rules).string = {min_bytes: 1}];
+
+    // used to populate the tag value if the name is not found in request header map.


Same comment here as above.

mattklein123 · 2019-09-27T17:06:10Z

api/envoy/type/tracing.proto

+message TracingCustomTag {
+  // Literal type custom tag with literal value for tag value.
+  message Literal {
+    // literal value to populate the tag value.


nit: Please use proper grammar for all doc comments including starting sentence with a capital letter. Same elsewhere.

mattklein123 · 2019-09-27T17:06:22Z

api/envoy/type/tracing.proto

+    // specify the separator for the full path.
+    string path_separator = 3;
+
+    // used to populate the tag value if no metadata found.


Same comment here as above.

include/envoy/router/router.h

mattklein123 · 2019-09-27T17:10:04Z

api/envoy/type/tracing.proto

+    string default_value = 2;
+  }
+
+  // Metadata type custom tag with namespace, path, and default value.


Can you flesh out the docs for this including potentially a full YAML example? It's confusing to me with these docs to understand how this actually works, what path is, path_separator, etc.

documented.

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

mattklein123

Thanks looks good. Few more comments.

/wait

source/common/http/conn_manager_impl.cc

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

htuch

@envoyproxy/api-shepherds API LGTM

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

mattklein123

Thanks, LGTM with 2 small comments.

/wait

mattklein123 · 2019-11-29T21:07:44Z

test/extensions/filters/network/http_connection_manager/config_test.cc

@@ -33,6 +33,14 @@ parseHttpConnectionManagerFromV2Yaml(const std::string& yaml) {
  return http_connection_manager;
 }

+envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager
+parseHttpConnectionManagerFromV2YamlAndValidate(const std::string& yaml) {


Sorry I meant for you to just change the previous parseHttpConnectionManagerFromV2Yaml to always validate vs. add a new function. Can you do that?

No, I can't, or it should be done in another PR, some other case will fail by validating

OK no problem, let's do this in a follow up please. Can you add a TODO?

mattklein123 · 2019-11-29T21:08:49Z

source/common/http/conn_manager_impl.cc

@@ -1346,15 +1346,15 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedTracingCustomTags() {
  if (!connection_manager_.config_.tracingConfig()) {
    return;
  }
-  Tracing::CustomTagMap& customTagMap = getOrMakeTracingCustomTagMap();
+  Tracing::CustomTagMap& custom_tag_map = getOrMakeTracingCustomTagMap();


In many cases there will be no custom tags. Can we avoid creating the map until we know there are custom tags on the route or HCM level below?

Ok I did like it before IIRC :), I'll add it back.
PS, checking the tracing config before all even though there are custom tags on the route level is needed, the tracing will be processed only when tracing in HCM configured.

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

mattklein123

Awesome work. 2 small comments and let's ship!

/wait

mattklein123 · 2019-12-02T16:47:33Z

source/common/http/conn_manager_impl.cc

+  if (hasCachedRoute() && cached_route_.value()->tracingConfig()) {
+    route_tags = &cached_route_.value()->tracingConfig()->getCustomTags();
+  }
+  bool configured_in_conn = !conn_manager_tags.empty();


nit: const here and next line

mattklein123 · 2019-12-02T16:51:56Z

test/extensions/filters/network/http_connection_manager/config_test.cc

@@ -33,6 +33,14 @@ parseHttpConnectionManagerFromV2Yaml(const std::string& yaml) {
  return http_connection_manager;
 }

+envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager
+parseHttpConnectionManagerFromV2YamlAndValidate(const std::string& yaml) {


OK no problem, let's do this in a follow up please. Can you add a TODO?

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

yittg · 2019-12-03T12:38:45Z

/retest

repokitteh-read-only · 2019-12-03T12:38:49Z

🙀 Error while processing event:

evaluation error
error: function error error: user: combined status is failure, but no failed builds.

🐱

Caused by: a #8279 (comment) was created by @yittg.

see: more, trace.

mattklein123

Thanks!

changes made

yittg · 2019-12-03T23:50:08Z

Thank you all for your help in the long way, :) .

objectiser · 2019-12-04T09:25:48Z

Great job @yittg !

objectiser reviewed Sep 18, 2019

View reviewed changes

zuercher assigned snowp Sep 18, 2019

snowp suggested changes Sep 18, 2019

View reviewed changes

yittg changed the title ~~tracing: enhance additional tags source~~ tracing: enhance custom tags source Sep 19, 2019

yittg commented Sep 19, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

yittg commented Sep 20, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

snowp previously requested changes Sep 20, 2019

View reviewed changes

objectiser reviewed Sep 23, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

test/common/http/conn_manager_impl_test.cc Outdated Show resolved Hide resolved

mattklein123 self-assigned this Sep 23, 2019

mattklein123 requested changes Sep 24, 2019

View reviewed changes

api/envoy/api/v2/route/route.proto Outdated Show resolved Hide resolved

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

repokitteh-read-only bot added the waiting label Sep 24, 2019

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

repokitteh-read-only bot added the waiting label Sep 25, 2019

mandarjog mentioned this pull request Sep 25, 2019

Add trace tags related to istio config objects istio/istio#17386

Closed

mandarjog reviewed Sep 25, 2019

View reviewed changes

api/envoy/type/tracing.proto Outdated Show resolved Hide resolved

yittg requested a review from alyssawilk as a code owner September 27, 2019 03:52

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

objectiser reviewed Sep 27, 2019

View reviewed changes

yittg closed this Sep 27, 2019

yittg reopened this Sep 27, 2019

mattklein123 requested changes Sep 27, 2019

View reviewed changes

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

fix

a34286e

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

mattklein123 requested changes Nov 26, 2019

View reviewed changes

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

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

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

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

htuch reviewed Nov 27, 2019

View reviewed changes

yittg added 2 commits November 28, 2019 14:51

Merge branch 'master' into tracing-additional-tags

58b201a

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

fix

18581c3

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 28, 2019

yittg added 4 commits November 28, 2019 18:35

fix

d2ff224

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

fix

376e35f

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

fix

bc115b1

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

fix

87990ef

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

mattklein123 requested changes Nov 29, 2019

View reviewed changes

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

fix

68d6788

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 30, 2019

mattklein123 requested changes Dec 2, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Dec 2, 2019

yittg added 2 commits December 3, 2019 13:50

Merge branch 'master' into tracing-additional-tags

7b67bfa

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

fix

e0cf3bb

Signed-off-by: Yi Tang <ssnailtang@gmail.com>

repokitteh-read-only bot removed the waiting label Dec 3, 2019

mattklein123 approved these changes Dec 3, 2019

View reviewed changes

repokitteh-read-only bot removed the api label Dec 3, 2019

mattklein123 merged commit 222890c into envoyproxy:master Dec 3, 2019

yittg deleted the tracing-additional-tags branch December 3, 2019 22:53

tracing: enhance custom tags source #8279

tracing: enhance custom tags source #8279

Conversation

yittg commented Sep 18, 2019 • edited

repokitteh-read-only bot commented Sep 18, 2019

objectiser left a comment

Choose a reason for hiding this comment

yittg commented Sep 18, 2019

zuercher commented Sep 18, 2019

snowp left a comment

Choose a reason for hiding this comment

yittg commented Sep 20, 2019

snowp left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

yittg commented Sep 25, 2019

mattklein123 commented Sep 25, 2019

kyessenov commented Sep 25, 2019

yittg commented Sep 26, 2019

objectiser 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

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

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

htuch left a 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

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

yittg commented Dec 3, 2019

repokitteh-read-only bot commented Dec 3, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

yittg commented Dec 3, 2019

objectiser commented Dec 4, 2019

yittg commented Sep 18, 2019 •

edited