tracing: Add hostname to Zipkin config. (#14186) #14187

esmet · 2020-11-25T16:51:47Z

This should be ready for review, though docs and release notes are still needed. Feedback on all fronts welcome.

Commit Message: tracing: Add hostname to Zipkin config
Additional Description:
Risk Level: low (new configuration, default behavior preserved)
Testing: unit (expanded an existing test)
Docs Changes: API protos documented, in line with existing fields
Release Notes: tracing: added support for setting the hostname used when sending spans to a Zipkin collector using the :ref:collector_hostname <envoy_v3_api_field_config.trace.v3.ZipkinConfig.collector_hostname> field.
Fixes #14186

repokitteh-read-only · 2020-11-25T16:51:50Z

Hi @esmet, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14187 was opened by esmet.

see: more, trace.

repokitteh-read-only · 2020-11-25T16:51:53Z

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

🐱

Caused by: #14187 was opened by esmet.

see: more, trace.

esmet · 2020-11-25T17:02:39Z

I force pushed an update to correct a DCO mistake per the guidelines.

mk46 · 2020-11-25T17:48:26Z

Need to fix the format. Run tools/code_format/check_format.py fix to fix it

esmet · 2020-11-25T17:53:04Z

tools/code_format/check_format.py fix

good call, done

dio

Looks good. Please provide release notes and docs. Thanks!

esmet · 2020-11-30T20:10:14Z

@dio the release notes were easy enough 👍

As for docs, it looks like we have a sample Envoy config ./examples/zipkin-tracing/front-envoy-zipkin.yaml but no additional documentation around the config itself. My understanding is that we typically rely on the API proto comments/docs for that. Is that right? And if not, what would you recommend?

dio · 2020-11-30T20:39:56Z

api/envoy/config/trace/v3/zipkin.proto

@@ -65,4 +65,8 @@ message ZipkinConfig {
  // Determines the selected collector endpoint version. By default, the ``HTTP_JSON_V1`` will be
  // used.
  CollectorEndpointVersion collector_endpoint_version = 5;
+
+  // Optional hostname to use when sending spans to the collector_cluster. Useful for collectors
+  // that require a specific hostname. Defaults to `collector_cluster` above.


We usually link this to the actual field.

Done. I did something similar to the link I added in the release notes, let me know if that's what you had in mind.

dio · 2020-12-03T11:47:27Z

Can you check CI? Seems like "verify examples" failed on Zipkin.

esmet · 2020-12-03T16:50:58Z

Can you check CI? Seems like "verify examples" failed on Zipkin.

I looked at this but can't figure out why it failed. I see some configuration errors though:

2020-12-02T22:25:27.9159539Z �[36mvrp_1  |�[0m (edge)[2020-12-02 22:25:17.813][11][critical][main] error initializing configuration '/etc/envoy/envoy-edge.yaml': Unable to parse JSON as proto (INVALID_ARGUMENT:(static_resources.clusters.typed_extension_protocol_options[0].value): invalid value Invalid type URL, unknown type: envoy.extensions.upstreams.http.v3.HttpProtocolOptions for type Any): {"static_resources":{"clusters":{"typed_extension_protocol_options":{"envoy.extensions.upstreams.http.v3.HttpProtocolOptions":{"explicit_http_config":{"http2_protocol_options":{"initial_connection_window_size":1048576,"initial_stream_window_size":65536}},"@type":"type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions"}},"connect_timeout":"5s","load_assignment":{"cluster_name":"service_foo","endpoints":[{"lb_endpoints":[{"endpoint":{"address":{"socket_address":{"port_value":10002,"address":"127.0.0.1"}}}}]}]},"per_connection_buffer_limit_bytes":32768,"name":"service_foo"},"listeners":[{"filter_chains":[{"filters":[{"typed_config":{"stream_idle_timeout":"300s","route_config":{"name":"local_route","virtual_hosts":[{"routes":[{"route":{"idle_timeout":"15s","cluster":"service_foo"},"match":{"prefix":"/content"}},{"direct_response":{"body":{"inline_string":"denied\n"},"status":403},"match":{"prefix":"/"}}],"name":"local_service","domains":["*"]}]},"use_remote_address":true,"common_http_protocol_options":{"idle_timeout":"3600s","headers_with_underscores_action":"REJECT_REQUEST"},"@type":"type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager","http2_protocol_options":{"max_concurrent_streams":100,"initial_connection_window_size":1048576,"initial_stream_window_size":65536},"request_timeout":"300s","http_filters":[{"name":"envoy.filters.http.router"}],"stat_prefix":"ingress_http"},"name":"envoy.filters.network.http_connection_manager"}],"transport_socket":{"typed_config":{"@type":"type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext","common_tls_context":{"tls_certificates":[{"private_key":{"filename":"certs/serverkey.pem"},"certificate_chain":{"filename":"certs/servercert.pem"}}]}},"name":"envoy.transport_sockets.tls"}}],"name":"listener_https","per_connection_buffer_limit_bytes":32768,"address":{"socket_address":{"address":"0.0.0.0","protocol":"TCP","port_value":10000}}}]},"overload_manager":{"resource_monitors":[{"name":"envoy.resource_monitors.fixed_heap","typed_config":{"@type":"type.googleapis.com/envoy.config.resource_monitor.fixed_heap.v2alpha.FixedHeapConfig","max_heap_size_bytes":1073741824}}],"refresh_interval":"0.25s","actions":[{"triggers":[{"name":"envoy.resource_monitors.fixed_heap","threshold":{"value":"0.90"}}],"name":"envoy.overload_actions.shrink_heap"},{"name":"envoy.overload_actions.stop_accepting_requests","triggers":[{"threshold":{"value":"0.95"},"name":"envoy.resource_monitors.fixed_heap"}]}]}}
...

I'll try rerunning to see if this reproduces.

Signed-off-by: John Esmet <[email protected]>

esmet · 2020-12-03T17:17:55Z

Can you check CI? Seems like "verify examples" failed on Zipkin.

I looked at this but can't figure out why it failed. I see some configuration errors though:

2020-12-02T22:25:27.9159539Z �[36mvrp_1  |�[0m (edge)[2020-12-02 22:25:17.813][11][critical][main] error initializing configuration '/etc/envoy/envoy-edge.yaml': Unable to parse JSON as proto (INVALID_ARGUMENT:(static_resources.clusters.typed_extension_protocol_options[0].value): invalid value Invalid type URL, unknown type: envoy.extensions.upstreams.http.v3.HttpProtocolOptions for type Any): {"static_resources":{"clusters":{"typed_extension_protocol_options":{"envoy.extensions.upstreams.http.v3.HttpProtocolOptions":{"explicit_http_config":{"http2_protocol_options":{"initial_connection_window_size":1048576,"initial_stream_window_size":65536}},"@type":"type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions"}},"connect_timeout":"5s","load_assignment":{"cluster_name":"service_foo","endpoints":[{"lb_endpoints":[{"endpoint":{"address":{"socket_address":{"port_value":10002,"address":"127.0.0.1"}}}}]}]},"per_connection_buffer_limit_bytes":32768,"name":"service_foo"},"listeners":[{"filter_chains":[{"filters":[{"typed_config":{"stream_idle_timeout":"300s","route_config":{"name":"local_route","virtual_hosts":[{"routes":[{"route":{"idle_timeout":"15s","cluster":"service_foo"},"match":{"prefix":"/content"}},{"direct_response":{"body":{"inline_string":"denied\n"},"status":403},"match":{"prefix":"/"}}],"name":"local_service","domains":["*"]}]},"use_remote_address":true,"common_http_protocol_options":{"idle_timeout":"3600s","headers_with_underscores_action":"REJECT_REQUEST"},"@type":"type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager","http2_protocol_options":{"max_concurrent_streams":100,"initial_connection_window_size":1048576,"initial_stream_window_size":65536},"request_timeout":"300s","http_filters":[{"name":"envoy.filters.http.router"}],"stat_prefix":"ingress_http"},"name":"envoy.filters.network.http_connection_manager"}],"transport_socket":{"typed_config":{"@type":"type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext","common_tls_context":{"tls_certificates":[{"private_key":{"filename":"certs/serverkey.pem"},"certificate_chain":{"filename":"certs/servercert.pem"}}]}},"name":"envoy.transport_sockets.tls"}}],"name":"listener_https","per_connection_buffer_limit_bytes":32768,"address":{"socket_address":{"address":"0.0.0.0","protocol":"TCP","port_value":10000}}}]},"overload_manager":{"resource_monitors":[{"name":"envoy.resource_monitors.fixed_heap","typed_config":{"@type":"type.googleapis.com/envoy.config.resource_monitor.fixed_heap.v2alpha.FixedHeapConfig","max_heap_size_bytes":1073741824}}],"refresh_interval":"0.25s","actions":[{"triggers":[{"name":"envoy.resource_monitors.fixed_heap","threshold":{"value":"0.90"}}],"name":"envoy.overload_actions.shrink_heap"},{"name":"envoy.overload_actions.stop_accepting_requests","triggers":[{"threshold":{"value":"0.95"},"name":"envoy.resource_monitors.fixed_heap"}]}]}}
...

I'll try rerunning to see if this reproduces.

I think this could be related to a merge to master that happened yesterday. I'll rebase. If CI goes green, @dio do you think the release notes change and API comments are enough to land this?

esmet · 2020-12-04T03:02:51Z

I rebased and CI looks good now. This should be ready for final review :)

dio

Thanks. Change looks good. However, this needs @envoyproxy/api-shepherds approval for API.

htuch

/lgtm api

* master: (41 commits) event: Remove a source of non-determinism by always running deferred deletion before post callbacks (envoyproxy#14293) Fix TSAN bug in integration test (envoyproxy#14327) tracing: Add hostname to Zipkin config. (envoyproxy#14186) (envoyproxy#14187) [conn_pool] fix use after free in H/1 connection pool (envoyproxy#14220) lua: update deprecated lua_open to luaL_newstate (envoyproxy#14297) extension: use bool_flag to control extension link (envoyproxy#14240) stats: Factor out creation of cluster-stats StatNames from creation of the stats, to save CPU during xDS updates (envoyproxy#14028) test: add scaled timer integration test (envoyproxy#14290) [Win32 Signals] Add term and ctrl-c signal handlers (envoyproxy#13954) config: v2 transport API fatal-by-default. (envoyproxy#14223) matcher: fix UB bug caused by dereferencing a bad optional (envoyproxy#14271) test: putting fake upstream config in a struct (envoyproxy#14266) wasm: use Bazel rules from Proxy-Wasm Rust SDK. (envoyproxy#14292) docs: fix typo (envoyproxy#14237) dependencies: allowlist CVE-2018-21270 to prevent false positives. (envoyproxy#14294) typo in redis doc (envoyproxy#14248) access_loggers: removed redundant dep (envoyproxy#14274) fix http2 flaky test (envoyproxy#14261) test: disable flaky xds_integration_test. (envoyproxy#14287) http: add functionality to configure kill header in KillRequest proto (envoyproxy#14288) ... Signed-off-by: Michael Puncel <[email protected]>

repokitteh-read-only bot added the api label Nov 25, 2020

esmet force-pushed the esmet/zipkin-hostname branch from 3e3d203 to 3be765d Compare November 25, 2020 17:00

zuercher assigned dio Nov 25, 2020

phlax mentioned this pull request Nov 27, 2020

Container will not start when deployed to OpenShift 3.11 with chmod /dev/stdout /dev/stderr permission error #14138

Closed

dio reviewed Nov 27, 2020

View reviewed changes

dio reviewed Nov 30, 2020

View reviewed changes

esmet force-pushed the esmet/zipkin-hostname branch from 1e34385 to bc0d00e Compare December 2, 2020 20:14

esmet added 6 commits December 3, 2020 17:12

zipkin: Add hostname to Zipkin config.

b8d56c9

Signed-off-by: John Esmet <[email protected]>

zipkin: tools/code_format/check_format.py fix

060d240

Signed-off-by: John Esmet <[email protected]>

tracing: add release notes for collector_hostnme in the zipkin config

4669129

Signed-off-by: John Esmet <[email protected]>

docs: try to fix the collector_hostname docs link in release notes

cf0f57e

Signed-off-by: John Esmet <[email protected]>

docs: try fixing it again

63e3a0a

Signed-off-by: John Esmet <[email protected]>

Add link to field in api/

0e5aef7

Signed-off-by: John Esmet <[email protected]>

esmet force-pushed the esmet/zipkin-hostname branch from 28a0d35 to 0e5aef7 Compare December 3, 2020 17:16

esmet changed the title ~~zipkin: Add hostname to Zipkin config. (#14186)~~ tracing: Add hostname to Zipkin config. (#14186) Dec 3, 2020

dio approved these changes Dec 7, 2020

View reviewed changes

htuch reviewed Dec 7, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Dec 7, 2020

mattklein123 merged commit 940958e into envoyproxy:master Dec 8, 2020

esmet deleted the esmet/zipkin-hostname branch December 8, 2020 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: Add hostname to Zipkin config. (#14186) #14187

tracing: Add hostname to Zipkin config. (#14186) #14187

esmet commented Nov 25, 2020 •

edited

Loading

repokitteh-read-only bot commented Nov 25, 2020

repokitteh-read-only bot commented Nov 25, 2020

esmet commented Nov 25, 2020

mk46 commented Nov 25, 2020

esmet commented Nov 25, 2020

dio left a comment

esmet commented Nov 30, 2020

dio Nov 30, 2020

esmet Nov 30, 2020

dio commented Dec 3, 2020

esmet commented Dec 3, 2020

esmet commented Dec 3, 2020 •

edited

Loading

esmet commented Dec 4, 2020

dio left a comment

htuch left a comment

tracing: Add hostname to Zipkin config. (#14186) #14187

tracing: Add hostname to Zipkin config. (#14186) #14187

Conversation

esmet commented Nov 25, 2020 • edited Loading

repokitteh-read-only bot commented Nov 25, 2020

repokitteh-read-only bot commented Nov 25, 2020

esmet commented Nov 25, 2020

mk46 commented Nov 25, 2020

esmet commented Nov 25, 2020

dio left a comment

Choose a reason for hiding this comment

esmet commented Nov 30, 2020

dio Nov 30, 2020

Choose a reason for hiding this comment

esmet Nov 30, 2020

Choose a reason for hiding this comment

dio commented Dec 3, 2020

esmet commented Dec 3, 2020

esmet commented Dec 3, 2020 • edited Loading

esmet commented Dec 4, 2020

dio left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

esmet commented Nov 25, 2020 •

edited

Loading

esmet commented Dec 3, 2020 •

edited

Loading