tracing: apply tracer provider configuration defined as part of `http_connection_manager` filter by yskopets · Pull Request #10405 · envoyproxy/envoy

yskopets · 2020-03-16T17:41:15Z

Description: apply tracer provider configuration defined as part of http_connection_manager filter
Risk Level: Low
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A

Context:

part of proposal: make configuration of a tracing provider dynamic #9998
~~depends on tracing: ensure that OpenCensus is configured at most once #10519~~

repokitteh-read-only · 2020-03-16T17:41:21Z

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

🐱

Caused by: #10405 was opened by yskopets.

see: more, trace.

yskopets · 2020-03-16T18:19:01Z

@mattklein123

I need your advice on how to better handle OpenCensus case.

Right now, I've documented why OpenCensus is different from other tracing providers.

Do you think we need something better than a warning in the docs ?

E.g., we could prohibit configuring *envoy.tracers.opencensus* via http_connection_manager and only allow it in the bootstrap config.

Also, current implementation of HttpTracerManager as a Singleton leaves a room for an error in case of OpenCensus. Since Singletons might be removed when no longer in use (no more Listeners left), it is possible that OpenCensus will be configured more than 1 time even if it's a part of bootstrap config. Unfortunately, OpenCensus will not handle gracefully double configuration (it will not crash Envoy, but its behaviour will be wrong, e.g. reporting a Span multiple times).

WDYT ?

…_connection_manager` filter Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

mattklein123 · 2020-03-17T17:08:46Z

E.g., we could prohibit configuring envoy.tracers.opencensus via http_connection_manager and only allow it in the bootstrap config.

If our goal is to get rid of tracing config in bootstrap (which I think it is) I don't think this is a viable long term option.

Unfortunately, OpenCensus will not handle gracefully double configuration (it will not crash Envoy, but its behaviour will be wrong, e.g. reporting a Span multiple times).

This seems like a pretty big bug in the OpenCensus implementation. Can we get this tracked somehow and someone working on an upstream fix? I would be inclined to just ignore this for now and let this be tracked as a known bug. IMO we should not go out of our way to work around broken extensions in Envoy. cc @g-easy @rnburn @objectiser

yskopets · 2020-03-23T22:02:40Z

@mattklein123

I don't want to leave any room for an error.

Because if it can happen, it will happen eventually, and it will cost somebody a few hours/days of troubleshooting.

So, I propose to update OpenCensusTracerFactory::createHttpTracer():

On first successful invocation, memorize the proto_config argument and a created Tracer instance
On subsequent invocations, compare proto_config argument against the first-time value; if they match, return previously created Tracer, otherwise throw an error

This way, HttpTracerManager can remain a Singleton and get deleted when it's necessary.

WDYT?

mattklein123 · 2020-03-23T22:35:52Z

WDYT?

Yup sounds like a great plan.

yskopets · 2020-03-23T22:47:17Z

@mattklein123

I will do it in a separate PR

…ion notice for tracing provider configuration as part of Bootstrap config Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

yskopets · 2020-03-25T18:14:59Z

@mattklein123

I've made a few changes:

rephrased warnings about OpenCensus (in the light of tracing: ensure that OpenCensus is configured at most once #10519)
added a deprecation notice for Bootstrap.tracing field

I was unable to add [deprecated = true] annotation, though:

if I add it to v2.Bootrstrap, then tracing field gets completely removed from v3.Bootrstrap
if I add it to v3.Bootrstrap, then it gets stripped off by automation on git push

How should I proceed ?

mattklein123 · 2020-03-25T20:17:06Z

How should I proceed ?

cc @htuch you can still access the hidden smuggled field in the v3 object (see the generated API protos that are checked in). However, technically we can't make breaking changes to v3 like this. We are likely going to lock v2 very soon (next ~1 week) and then we will be working on v3 and v4alpha. So what I would probably do is deprecate this field from v2 and get the hidden smuggled field approach working. Then when we move to v3/v4alpha it should be straightforward to move this change over.

…r-per-http-connection-manager Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

htuch · 2020-03-26T00:27:08Z

Yeah, I'm actively working on moving the v2->v3 API migration tooling to go v3->v4alpha, ETA < 1 week.

…acing` field Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

…PRECATED_FEATURE_TEST()` Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

…uilt with `--define deprecated_features=disabled` option Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

yskopets · 2020-03-26T15:49:09Z

@mattklein123

After adding [deprecated = true] to v2.Bootstrap.tracing field, //test/config_test:example_configs_test was failing in CI builds with --define deprecated_features=disabled option.

To fix that, I updated all "example_configs" to define tracing provider as part of HCM.

The only example left that is still using v2.Bootstrap.tracing field is configs/using_deprecated_config.v2.yaml.

Let met know if it's not what you expected.

mattklein123

Thanks in general this looks great. Let's revisit in ~1 week when we lock v2?

/wait

mattklein123 · 2020-03-26T17:48:19Z

    // If not specified, Envoy will fall back to using tracing provider configuration
-    // from the bootstrap config.
-    // [#not-implemented-hide:]
+    // from the bootstrap config (for short-term backwards compatibility).


I would make the new docs read as it should be now vs. referring back to the previous behavior. That can be covered in deprecated.rst.

mattklein123 · 2020-03-26T17:49:07Z

+// :ref:`provider <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.Tracing.provider>`
+// field.
+//
+// Alternatively, for the sake of short-term backwards compatibility, it is also possible to define


Same comment here. Please make the new docs refer to the expected new config, and talk about deprecation in deprecated.rst

mattklein123 · 2020-03-26T17:49:41Z

          typed_config: {}
        tracing:
          operation_name: INGRESS
+          # Notice that tracing provider configuration is now part of "envoy.filters.network.http_connection_manager" config.


nit: I think this comment can be skipped, same elsewhere.

…d in v2 api" This reverts commit 2107e1f. Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

yskopets · 2020-04-08T20:28:36Z

@mattklein123

When I refer to v3 apis in deprecated.rst and version_history.rst, CI build for docs fails with

/source/generated/rst/intro/deprecated.rst:15: WARNING: undefined label: envoy_api_field_config.bootstrap.v3.bootstrap.tracing (if the link has no caption the label must precede a section header)
/source/generated/rst/intro/deprecated.rst:15: WARNING: undefined label: envoy_api_field_extensions.filters.network.http_connection_manager.v3.httpconnectionmanager.tracing.provider (if the link has no caption the label must precede a section header)
/source/generated/rst/intro/version_history.rst:6: WARNING: undefined label: envoy_api_field_config.bootstrap.v3.bootstrap.tracing (if the link has no caption the label must precede a section header)
/source/generated/rst/intro/version_history.rst:6: WARNING: undefined label: envoy_api_field_extensions.filters.network.http_connection_manager.v3.httpconnectionmanager.tracing.provider (if the link has no caption the label must precede a section header)

How should I fix that ?

mattklein123 · 2020-04-08T20:33:56Z

@htuch see question about doc failure above?

htuch · 2020-04-08T23:06:03Z

@yskopets try using envoy_v3_api... prefix for the v3 protos, e.g. envoy_v3_api_field_config.bootstrap.v3.Bootstrap.tracing

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

yskopets · 2020-04-08T23:20:52Z

@htuch thanks! it helped

mattklein123

Thanks this is a great improvement. Just a few small questions/comments.

/wait

mattklein123 · 2020-04-09T23:32:47Z

+//
+// Envoy may support other tracers in the future, but right now the HTTP tracer is the only one
+// supported.
 message Tracing {


Is this message actually used anymore or just the inner HTTP message? Should this message actually be tagged for deletion with the inner message moved somehow? I don't think the tool supports this but we might want to file an issue here? cc @htuch

config.trace.v3.Tracing will not be used anymore in v4.

I don't see a support for marking the entire message type as deprecated. The closest example is v2.Bootstrap.Runtime which only has (deprecated) comment in the docs.

I've added an "attention" message to config.trace.v3.Tracing for now.

@htuch Should I open an issue or you can phrase it better what needs to be done ?

Please open an issue to track this. We will need some tooling work I think to make sure this message gets deleted/moved/etc. I'm not sure the best way of handling.

opened #10740

mattklein123 · 2020-04-09T23:33:47Z


 1.15.0 (Pending)
 ================
+* tracing: tracing provider configuration as part of :ref:`bootstrap config <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.tracing>`


nit: I wouldn't discuss deprecation here but rather just describe the new way of configuring it. You describe deprecation elsewhere.

mattklein123 · 2020-04-09T23:35:48Z

+TEST_P(ServerInstanceImplTest, DEPRECATED_FEATURE_TEST(ZipkinHttpTracingEnabled)) {
  options_.service_cluster_name_ = "some_cluster_name";
  options_.service_node_name_ = "some_node_name";
  EXPECT_NO_THROW(initialize("test/server/test_data/server/zipkin_tracing.yaml"));


nit: can you rename this zipkin_tracing_deprecated_config or similar so it's clear that it should be removed when this is cleaned up?

…_data/server/zipkin_tracing_deprecated_config.yaml` Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

htuch · 2020-04-10T15:10:55Z

+  // .. attention::
+  //  This field has been deprecated in favor of :ref:`HttpConnectionManager.Tracing.provider
+  //  <envoy_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.Tracing.provider>`.
+  trace.v3.Tracing tracing = 9 [deprecated = true];


One question before deprecating, do you have thoughts on #10576?

@htuch Sorry, didn't notice in time.

I've replied here.

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

mattklein123

LGTM, thanks!

mattklein123 · 2020-04-10T19:31:36Z

Apologies needs a master merge. We moved version history files around (see master).

/wait

…r-per-http-connection-manager Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

mattklein123

Thanks!

yskopets requested review from alyssawilk and mattklein123 as code owners March 16, 2020 17:41

repokitteh-read-only Bot added the api label Mar 16, 2020

yskopets force-pushed the feature/use-tracer-per-http-connection-manager branch from 08f2a00 to 52a48c6 Compare March 16, 2020 17:51

mattklein123 self-assigned this Mar 16, 2020

yskopets force-pushed the feature/use-tracer-per-http-connection-manager branch from 52a48c6 to a6be7b9 Compare March 16, 2020 18:06

yskopets force-pushed the feature/use-tracer-per-http-connection-manager branch from a6be7b9 to af52fbb Compare March 16, 2020 18:30

tracing: apply tracer provider configuration defined as part of `http…

0561ccc

…_connection_manager` filter Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

yskopets force-pushed the feature/use-tracer-per-http-connection-manager branch from af52fbb to 0561ccc Compare March 16, 2020 23:58

mattklein123 added the waiting label Mar 17, 2020

docs: update docs with regards to OpenCensus support and add deprecat…

6ba1f98

…ion notice for tracing provider configuration as part of Bootstrap config Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

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

mattklein123 added the waiting label Mar 25, 2020

Merge remote-tracking branch 'upstream/master' into feature/use-trace…

0e36c6c

…r-per-http-connection-manager Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

code review: add [deprecated = true] annotation to `v2.Bootstrap.tr…

b910b95

…acing` field Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

repokitteh-read-only Bot removed the waiting label Mar 26, 2020

yskopets added 3 commits March 26, 2020 12:20

docs: update tracing-related proto docs

7ca80c7

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

unit tests: mark unit tests the involve v2.Bootstrap.tracing as `DE…

41013a4

…PRECATED_FEATURE_TEST()` Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

unit tests: update example configurations to test fix failures when b…

073b24e

…uilt with `--define deprecated_features=disabled` option Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

mattklein123 requested changes Mar 26, 2020

View reviewed changes

yskopets added 2 commits April 8, 2020 22:03

Revert "code review: leave HCM.Tracing.provider field as unimplmenete…

834740f

…d in v2 api" This reverts commit 2107e1f. Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

code review: update docs for Bootstrap.tracing field in v2 api

16722dc

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

yskopets requested a review from mattklein123 April 8, 2020 20:16

mattklein123 added the waiting label Apr 8, 2020

ci: try to fix failing docs build

220f429

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

repokitteh-read-only Bot removed the waiting label Apr 8, 2020

mattklein123 requested changes Apr 9, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Apr 9, 2020

code review: rename test_data/server/zipkin_tracing.yaml into `test…

67668e4

…_data/server/zipkin_tracing_deprecated_config.yaml` Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

htuch reviewed Apr 10, 2020

View reviewed changes

yskopets added 2 commits April 10, 2020 17:26

code review: reword release notes

b5348f9

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

code review: depreate use of config.trace.v3.Tracing

6642087

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

repokitteh-read-only Bot removed the waiting label Apr 10, 2020

ci: fix cross-link in the docs

ed40fac

Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

mattklein123 previously approved these changes Apr 10, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label Apr 10, 2020

repokitteh-read-only Bot added the waiting label Apr 10, 2020

Merge remote-tracking branch 'upstream/master' into feature/use-trace…

e88c14f

…r-per-http-connection-manager Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>

yskopets dismissed mattklein123’s stale review via e88c14f April 10, 2020 19:55

repokitteh-read-only Bot added api and removed waiting labels Apr 10, 2020

yskopets mentioned this pull request Apr 10, 2020

tools/api: add support for 1) deprecating message types and 2) moving nested message types up #10740

Open

mattklein123 approved these changes Apr 10, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label Apr 10, 2020

mattklein123 merged commit 449be5e into envoyproxy:master Apr 10, 2020

Conversation

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

Uh oh!

Uh oh!

repokitteh-read-only Bot commented Mar 16, 2020

Uh oh!

yskopets commented Mar 16, 2020

Uh oh!

mattklein123 commented Mar 17, 2020

Uh oh!

yskopets commented Mar 23, 2020

Uh oh!

mattklein123 commented Mar 23, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mattklein123 commented Mar 25, 2020

Uh oh!

htuch commented Mar 26, 2020

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

yskopets commented Apr 8, 2020

Uh oh!

mattklein123 commented Apr 8, 2020

Uh oh!

htuch commented Apr 8, 2020

Uh oh!

yskopets commented Apr 8, 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!

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

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Apr 10, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

yskopets commented Mar 16, 2020 •

edited

Loading

yskopets commented Mar 23, 2020 •

edited

Loading

yskopets commented Mar 25, 2020 •

edited

Loading