Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracing: apply tracer provider configuration defined as part of http_connection_manager filter #10405

Merged
merged 24 commits into from
Apr 10, 2020
Merged

tracing: apply tracer provider configuration defined as part of http_connection_manager filter #10405

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0561ccc
tracing: apply tracer provider configuration defined as part of `http…
yskopets Mar 16, 2020
6ba1f98
docs: update docs with regards to OpenCensus support and add deprecat…
yskopets Mar 25, 2020
0e36c6c
Merge remote-tracking branch 'upstream/master' into feature/use-trace…
yskopets Mar 25, 2020
b910b95
code review: add `[deprecated = true]` annotation to `v2.Bootstrap.tr…
yskopets Mar 25, 2020
7ca80c7
docs: update tracing-related proto docs
yskopets Mar 26, 2020
41013a4
unit tests: mark unit tests the involve `v2.Bootstrap.tracing` as `DE…
yskopets Mar 26, 2020
073b24e
unit tests: update example configurations to test fix failures when b…
yskopets Mar 26, 2020
0801eba
code review: remove extra comments
yskopets Apr 8, 2020
880824d
code review: rework comment
yskopets Apr 8, 2020
f079135
code review: rework comment
yskopets Apr 8, 2020
49e0ec4
Merge remote-tracking branch 'upstream/master' into feature/use-trace…
yskopets Apr 8, 2020
66b1118
v2 api freeze: rework deprecation of Bootstrap.tracing field
yskopets Apr 8, 2020
fbc2921
Merge remote-tracking branch 'upstream/master' into feature/use-trace…
yskopets Apr 8, 2020
6bb0bdb
code review: move deprecation and release notices into 1.15.0 release
yskopets Apr 8, 2020
fc6591d
code review: do not deprecate Bootstrap.tracing field in v2 api
yskopets Apr 8, 2020
2107e1f
code review: leave HCM.Tracing.provider field as unimplmeneted in v2 api
yskopets Apr 8, 2020
834740f
Revert "code review: leave HCM.Tracing.provider field as unimplmenete…
yskopets Apr 8, 2020
16722dc
code review: update docs for Bootstrap.tracing field in v2 api
yskopets Apr 8, 2020
220f429
ci: try to fix failing `docs` build
yskopets Apr 8, 2020
67668e4
code review: rename `test_data/server/zipkin_tracing.yaml` into `test…
yskopets Apr 10, 2020
b5348f9
code review: reword release notes
yskopets Apr 10, 2020
6642087
code review: depreate use of `config.trace.v3.Tracing`
yskopets Apr 10, 2020
ed40fac
ci: fix cross-link in the docs
yskopets Apr 10, 2020
e88c14f
Merge remote-tracking branch 'upstream/master' into feature/use-trace…
yskopets Apr 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions api/envoy/config/bootstrap/v2/bootstrap.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,12 @@ message Bootstrap {
// Optional watchdog configuration.
Watchdog watchdog = 8;

// Configuration for an external tracing provider. If not specified, no
// tracing will be performed.
trace.v2.Tracing tracing = 9;
// Configuration for an external tracing provider.
//
// .. attention::
// This field has been deprecated in favor of :ref:`HttpConnectionManager.Tracing.provider
// <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.Tracing.provider>`.
trace.v2.Tracing tracing = 9 [deprecated = true];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v2 is frozen so this need to move to v3. Please merge master.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I at least update the doc ?

Otherwise, the doc will be confusing. Currently, it says:

// If not specified, no tracing will be performed.

It's not possible to understand from it that a configuration inside HCM is enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the doc fix is fine, thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


// Configuration for the runtime configuration provider (deprecated). If not
// specified, a “null” provider will be used which will result in all defaults
Expand Down
1 change: 0 additions & 1 deletion api/envoy/config/bootstrap/v3/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ api_proto_package(
"//envoy/config/listener/v3:pkg",
"//envoy/config/metrics/v3:pkg",
"//envoy/config/overload/v3:pkg",
"//envoy/config/trace/v3:pkg",
"//envoy/extensions/transport_sockets/tls/v3:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
],
Expand Down
9 changes: 2 additions & 7 deletions api/envoy/config/bootstrap/v3/bootstrap.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import "envoy/config/core/v3/socket_option.proto";
import "envoy/config/listener/v3/listener.proto";
import "envoy/config/metrics/v3/stats.proto";
import "envoy/config/overload/v3/overload.proto";
import "envoy/config/trace/v3/trace.proto";
import "envoy/extensions/transport_sockets/tls/v3/cert.proto";

import "google/protobuf/duration.proto";
Expand Down Expand Up @@ -84,9 +83,9 @@ message Bootstrap {
core.v3.ApiConfigSource ads_config = 3;
}

reserved 10, 11;
reserved 10, 9, 11;

reserved "runtime";
reserved "tracing", "runtime";

// Node identity to present to the management server and for instance
// identification purposes (e.g. in generated headers).
Expand Down Expand Up @@ -128,10 +127,6 @@ message Bootstrap {
// Optional watchdog configuration.
Watchdog watchdog = 8;

// Configuration for an external tracing provider. If not specified, no
// tracing will be performed.
trace.v3.Tracing tracing = 9;

// Configuration for the runtime configuration provider. If not
// specified, a “null” provider will be used which will result in all defaults
// being used.
Expand Down
14 changes: 12 additions & 2 deletions api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,18 @@ message HttpConnectionManager {

// Configuration for an external tracing provider.
// 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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

//
// The advantage of using this field over relying on the bootstrap config is that now
// tracing provider configuration can be completely dynamic and get delivered over xDS.
//
// .. attention::
// Please be aware that *envoy.tracers.opencensus* provider can only be configured once
// in Envoy lifetime.
// Any attempts to reconfigure it or to use different configurations for different HCM filters
// will be rejected.
// Such a constraint is inherent to OpenCensus itself. It cannot be overcome without changes
// on OpenCensus side.
trace.v2.Tracing.Http provider = 9;
}

Expand Down
21 changes: 16 additions & 5 deletions api/envoy/config/trace/v2/trace.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,22 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// [#protodoc-title: Tracing]
// Tracing :ref:`architecture overview <arch_overview_tracing>`.

// The tracing configuration specifies global
// settings for the HTTP tracer used by Envoy. The configuration is defined by
// the :ref:`Bootstrap <envoy_api_msg_config.bootstrap.v2.Bootstrap>` :ref:`tracing
// <envoy_api_field_config.bootstrap.v2.Bootstrap.tracing>` field. Envoy may support other tracers
// in the future, but right now the HTTP tracer is the only one supported.
// The tracing configuration specifies settings for an HTTP tracer provider used by Envoy.
//
// The configuration should preferably be defined as part of
// the :ref:`HttpConnectionManager.Tracing <envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager.Tracing>`
// :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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

// tracing configuration as part of the :ref:`Bootstrap.tracing <envoy_api_msg_config.bootstrap.v2.Bootstrap>`
// field.
//
// [#comment:TODO(yskopets): Restore the link to the Bootstrap.tracing field after the switch to
// v3/v4alpha api cycle: :ref:`tracing <envoy_api_field_config.bootstrap.v2.Bootstrap.tracing>`]
//
// Envoy may support other tracers in the future, but right now the HTTP tracer is the only one
// supported.
message Tracing {
message Http {
// The name of the HTTP trace driver to instantiate. The name must match a
Expand Down
21 changes: 16 additions & 5 deletions api/envoy/config/trace/v3/trace.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,22 @@ option (udpa.annotations.file_status).package_version_status = NEXT_MAJOR_VERSIO
// [#protodoc-title: Tracing]
// Tracing :ref:`architecture overview <arch_overview_tracing>`.

// The tracing configuration specifies global
// settings for the HTTP tracer used by Envoy. The configuration is defined by
// the :ref:`Bootstrap <envoy_api_msg_config.bootstrap.v3.Bootstrap>` :ref:`tracing
// <envoy_api_field_config.bootstrap.v3.Bootstrap.tracing>` field. Envoy may support other tracers
// in the future, but right now the HTTP tracer is the only one supported.
// The tracing configuration specifies settings for an HTTP tracer provider used by Envoy.
//
// The configuration should preferably be defined as part of
// the :ref:`HttpConnectionManager.Tracing <envoy_api_msg_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.Tracing>`
// :ref:`provider <envoy_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.Tracing.provider>`
// field.
//
// Alternatively, for the sake of short-term backwards compatibility, it is also possible to define
// tracing configuration as part of the :ref:`Bootstrap.tracing <envoy_api_msg_config.bootstrap.v3.Bootstrap>`
// field.
//
// [#comment:TODO(yskopets): Restore the link to the Bootstrap.tracing field after the switch to
// v3/v4alpha api cycle: :ref:`tracing <envoy_api_field_config.bootstrap.v3.Bootstrap.tracing>`]
//
// Envoy may support other tracers in the future, but right now the HTTP tracer is the only one
// supported.
message Tracing {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opened #10740

option (udpa.annotations.versioning).previous_message_type = "envoy.config.trace.v2.Tracing";

Expand Down
14 changes: 12 additions & 2 deletions ...envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,18 @@ message HttpConnectionManager {

// Configuration for an external tracing provider.
// 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).
//
// The advantage of using this field over relying on the bootstrap config is that now
// tracing provider configuration can be completely dynamic and get delivered over xDS.
//
// .. attention::
// Please be aware that *envoy.tracers.opencensus* provider can only be configured once
// in Envoy lifetime.
// Any attempts to reconfigure it or to use different configurations for different HCM filters
// will be rejected.
// Such a constraint is inherent to OpenCensus itself. It cannot be overcome without changes
// on OpenCensus side.
config.trace.v3.Tracing.Http provider = 9;
}

Expand Down
14 changes: 7 additions & 7 deletions configs/envoy_double_proxy_v2.template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@
typed_config: {}
tracing:
operation_name: INGRESS
# Notice that tracing provider configuration is now part of "envoy.filters.network.http_connection_manager" config.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

provider:
name: envoy.tracers.lightstep
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.LightstepConfig
access_token_file: "/etc/envoy/lightstep_access_token"
collector_cluster: lightstep_saas
common_http_protocol_options:
idle_timeout: 840s
access_log:
Expand Down Expand Up @@ -177,13 +184,6 @@ stats_sinks:
typed_config:
"@type": type.googleapis.com/envoy.config.metrics.v2.StatsdSink
tcp_cluster_name: statsd
tracing:
http:
name: envoy.tracers.lightstep
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.LightstepConfig
access_token_file: "/etc/envoy/lightstep_access_token"
collector_cluster: lightstep_saas
layered_runtime:
layers:
- name: root
Expand Down
14 changes: 7 additions & 7 deletions configs/envoy_front_proxy_v2.template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@
add_user_agent: true
tracing:
operation_name: INGRESS
# Notice that tracing provider configuration is now part of "envoy.filters.network.http_connection_manager" config.
provider:
name: envoy.tracers.lightstep
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.LightstepConfig
collector_cluster: lightstep_saas
access_token_file: "/etc/envoy/lightstep_access_token"
common_http_protocol_options:
idle_timeout: 840s
access_log:
Expand Down Expand Up @@ -155,13 +162,6 @@ cluster_manager:
outlier_detection:
event_log_path: /var/log/envoy/outlier_events.log
flags_path: /etc/envoy/flags
tracing:
http:
name: envoy.tracers.lightstep
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.LightstepConfig
collector_cluster: lightstep_saas
access_token_file: "/etc/envoy/lightstep_access_token"
layered_runtime:
layers:
- name: root
Expand Down
7 changes: 0 additions & 7 deletions configs/envoy_service_to_service_v2.template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,13 +551,6 @@ stats_sinks:
"@type": type.googleapis.com/envoy.config.metrics.v2.StatsdSink
tcp_cluster_name: statsd
watchdog: {}
tracing:
http:
name: envoy.tracers.lightstep
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.LightstepConfig
access_token_file: "/etc/envoy/lightstep_access_token"
collector_cluster: lightstep_saas
layered_runtime:
layers:
- name: root
Expand Down
3 changes: 3 additions & 0 deletions docs/root/intro/deprecated.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ Deprecated items below are listed in chronological order.
has been deprecated in favor of `header_compressor_used`.
* Support for the undocumented HTTP/1.1 `:no-chunks` pseudo-header has been removed. If an extension
was using this it can achieve the same behavior via the new `http1StreamEncoderOptions()` API.
* Tracing provider configuration as part of :ref:`bootstrap config <envoy_api_field_config.bootstrap.v2.Bootstrap.tracing>`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move to 1.15.0. Also I think this changes needs a release note.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

has been deprecated in favor of configuration as part of :ref:`HTTP connection manager
<envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.Tracing.provider>`.

1.13.0 (January 20, 2020)
=========================
Expand Down
41 changes: 21 additions & 20 deletions examples/jaeger-native-tracing/front-envoy-jaeger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,27 @@ static_resources:
typed_config:
"@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
generate_request_id: true
tracing:
# Notice that tracing provider configuration is now part of "envoy.filters.network.http_connection_manager" config.
provider:
name: envoy.tracers.dynamic_ot
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig
library: /usr/local/lib/libjaegertracing_plugin.so
config:
service_name: front-proxy
sampler:
type: const
param: 1
reporter:
localAgentHostPort: jaeger:6831
headers:
jaegerDebugHeader: jaeger-debug-id
jaegerBaggageHeader: jaeger-baggage
traceBaggageHeaderPrefix: uberctx-
baggage_restrictions:
denyBaggageOnInitializationFailure: false
hostPort: ""
codec_type: auto
stat_prefix: ingress_http
route_config:
Expand Down Expand Up @@ -45,26 +66,6 @@ static_resources:
socket_address:
address: service1
port_value: 80
tracing:
http:
name: envoy.tracers.dynamic_ot
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig
library: /usr/local/lib/libjaegertracing_plugin.so
config:
service_name: front-proxy
sampler:
type: const
param: 1
reporter:
localAgentHostPort: jaeger:6831
headers:
jaegerDebugHeader: jaeger-debug-id
jaegerBaggageHeader: jaeger-baggage
traceBaggageHeaderPrefix: uberctx-
baggage_restrictions:
denyBaggageOnInitializationFailure: false
hostPort: ""
admin:
access_log_path: "/dev/null"
address:
Expand Down
41 changes: 21 additions & 20 deletions examples/jaeger-native-tracing/service1-envoy-jaeger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,27 @@ static_resources:
- name: envoy.filters.network.http_connection_manager
typed_config:
"@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
tracing:
# Notice that tracing provider configuration is now part of "envoy.filters.network.http_connection_manager" config.
provider:
name: envoy.tracers.dynamic_ot
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig
library: /usr/local/lib/libjaegertracing_plugin.so
config:
service_name: service1
sampler:
type: const
param: 1
reporter:
localAgentHostPort: jaeger:6831
headers:
jaegerDebugHeader: jaeger-debug-id
jaegerBaggageHeader: jaeger-baggage
traceBaggageHeaderPrefix: uberctx-
baggage_restrictions:
denyBaggageOnInitializationFailure: false
hostPort: ""
codec_type: auto
stat_prefix: egress_http
route_config:
Expand Down Expand Up @@ -84,26 +105,6 @@ static_resources:
socket_address:
address: service2
port_value: 80
tracing:
http:
name: envoy.tracers.dynamic_ot
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig
library: /usr/local/lib/libjaegertracing_plugin.so
config:
service_name: service1
sampler:
type: const
param: 1
reporter:
localAgentHostPort: jaeger:6831
headers:
jaegerDebugHeader: jaeger-debug-id
jaegerBaggageHeader: jaeger-baggage
traceBaggageHeaderPrefix: uberctx-
baggage_restrictions:
denyBaggageOnInitializationFailure: false
hostPort: ""
admin:
access_log_path: "/dev/null"
address:
Expand Down
41 changes: 21 additions & 20 deletions examples/jaeger-native-tracing/service2-envoy-jaeger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,27 @@ static_resources:
- name: envoy.filters.network.http_connection_manager
typed_config:
"@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
tracing:
# Notice that tracing provider configuration is now part of "envoy.filters.network.http_connection_manager" config.
provider:
name: envoy.tracers.dynamic_ot
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig
library: /usr/local/lib/libjaegertracing_plugin.so
config:
service_name: service2
sampler:
type: const
param: 1
reporter:
localAgentHostPort: jaeger:6831
headers:
jaegerDebugHeader: jaeger-debug-id
jaegerBaggageHeader: jaeger-baggage
traceBaggageHeaderPrefix: uberctx-
baggage_restrictions:
denyBaggageOnInitializationFailure: false
hostPort: ""
codec_type: auto
stat_prefix: ingress_http
route_config:
Expand Down Expand Up @@ -42,26 +63,6 @@ static_resources:
socket_address:
address: 127.0.0.1
port_value: 8080
tracing:
http:
name: envoy.tracers.dynamic_ot
typed_config:
"@type": type.googleapis.com/envoy.config.trace.v2.DynamicOtConfig
library: /usr/local/lib/libjaegertracing_plugin.so
config:
service_name: service2
sampler:
type: const
param: 1
reporter:
localAgentHostPort: jaeger:6831
headers:
jaegerDebugHeader: jaeger-debug-id
jaegerBaggageHeader: jaeger-baggage
traceBaggageHeaderPrefix: uberctx-
baggage_restrictions:
denyBaggageOnInitializationFailure: false
hostPort: ""
admin:
access_log_path: "/dev/null"
address:
Expand Down
Loading