tracing: Enable decorated operation in outbound (egress) listener to … #1858

objectiser · 2017-10-15T16:48:47Z

…be passed to inbound (ingress) listener to override server span operation

Resolves #1849

Signed-off-by: Gary Brown gary@brownuk.com

…be passed to inbound (ingress) listener to override server span operation Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 · 2017-10-16T00:44:27Z

@objectiser quick pre-review note that we will need docs for this change. It would be good to add this header into the router headers section that I mentioned in the issue, and also link to their from the tracing docs. Thank you.

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 · 2017-10-16T16:31:21Z

docs/configuration/http_filters/router_filter.rst

+x-envoy-decorator-operation
+^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+If this header is present on ingress requests, it's value will override any locally defined


mattklein123 · 2017-10-16T16:32:18Z

include/envoy/http/header_map.h

@@ -234,6 +234,7 @@ class HeaderEntry {
  HEADER_FUNC(EnvoyUpstreamRequestTimeoutAltResponse)                                              \
  HEADER_FUNC(EnvoyUpstreamRequestTimeoutMs)                                                       \
  HEADER_FUNC(EnvoyUpstreamServiceTime)                                                            \
+  HEADER_FUNC(EnvoyDecoratorOperation)                                                             \


nit: Alpha order

mattklein123 · 2017-10-16T16:33:23Z

include/envoy/router/router.h

+   * This method returns the operation name.
+   * @return the operation name
+   */
+  virtual const std::string getOperation() const PURE;


const std::string&

mattklein123 · 2017-10-16T16:34:20Z

source/common/http/conn_manager_impl.cc

+  ConnectionManagerImpl::chargeTracingStats(tracing_decision.reason,
+                                            connection_manager_.config_.tracingStats());
+
+  if (tracing_decision.is_tracing) {


nit: Now that you are in a new function, to reduce nesting can you do:

if (!tracing_decision.is_tracing) { return; }

mattklein123 · 2017-10-16T16:36:52Z

source/common/http/conn_manager_impl.cc

+    active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_);
+    if (cached_route_.value() && cached_route_.value()->decorator()) {
+      cached_route_.value()->decorator()->apply(*active_span_);
+      if (connection_manager_.config_.tracingConfig()->operation_name_ ==


I didn't realize the header propagation was also part of this. Can you make that more clear in the docs. Also, can you add some comments here about the logic in place? It's a little hard to quickly untangle what we are doing and why.

mattklein123 · 2017-10-16T16:38:39Z

docs/configuration/http_conn_man/header_sanitizing.rst

@@ -12,6 +12,7 @@ depends on the :ref:`use_remote_address <config_http_conn_man_use_remote_address

 Envoy will potentially sanitize the following headers:

+* :ref:`x-envoy-decorator-operation <config_http_filters_router_x-envoy-decorator-operation>`


I think this needs to be implemented? (Remove header on external requests, need to add test for that also in conn_manager_utility)

Signed-off-by: Gary Brown <gary@brownuk.com>

rshriram · 2017-10-17T17:19:48Z

source/common/http/conn_manager_impl.cc

+  if (request_headers_->EnvoyDecoratorOperation()) {
+    decorator_operation = request_headers_->EnvoyDecoratorOperation()->value().c_str();
+  }
+


so, there is some work going on in Istio to be able to decode a request using swagger spec/openapi spec.. And if that is the case, one might want to set the operation name based on the API spec. Would it make sense to have an interface for a filter to overwrite the operation name, in addition to the inbound header name doing the full override?

I think that makes sense, but let's track in a separate issue?

rshriram

Thanks for doing this!

mattklein123

Looks good. Few small things.

mattklein123 · 2017-10-17T18:06:23Z

source/common/http/conn_manager_impl.cc

+
+  active_span_ = connection_manager_.tracer_.startSpan(*this, *request_headers_, request_info_);
+
+  // If a decorator has been defined, apply it to the active span


super nit: please end sentences with '.'. Here and below.

should be sorted.

mattklein123 · 2017-10-17T18:08:37Z

source/common/http/conn_manager_impl.cc

@@ -502,6 +502,11 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
    state_.saw_connection_close_ = true;
  }

+  std::string decorator_operation;


Instead of allocating a string here on the stack, it would be faster to just pass the pointer value of the header entry into the trace function as you did before. Any reason to switch it?

I got a core dump when running the coverage tests - although strangely not when doing the bazel.dev build which also runs the tests. The problem seems to be caused by the header entry being removed/unallocated.

[----------] 38 tests from HttpConnectionManagerImplTest
[ RUN ] HttpConnectionManagerImplTest.HeaderOnlyRequestAndResponse
[ OK ] HttpConnectionManagerImplTest.HeaderOnlyRequestAndResponse (2 ms)
[ RUN ] HttpConnectionManagerImplTest.InvalidPathWithDualFilter
[ OK ] HttpConnectionManagerImplTest.InvalidPathWithDualFilter (1 ms)
[ RUN ] HttpConnectionManagerImplTest.StartAndFinishSpanNormalFlow
pure virtual method called
terminate called without an active exception
[2017-10-17 19:29:59.086][2295][critical][backtrace] bazel-out/local-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:101] Caught Aborted, suspect faulting address 0x3e8000008f7
[2017-10-17 19:29:59.086][2295][critical][backtrace] bazel-out/local-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:85] Backtrace obj</lib/x86_64-linux-gnu/libc.so.6> thr<0> (use tools/stack_decode.py):
[2017-10-17 19:29:59.086][2295][critical][backtrace] bazel-out/local-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:95] thr<0> #0 0x7f26ff0d0428
[2017-10-17 19:29:59.086][2295][critical][backtrace] bazel-out/local-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:95] thr<0> #1 0x7f26ff0d2029
[2017-10-17 19:29:59.086][2295][critical][backtrace] bazel-out/local-dbg/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:93] thr<0> obj</usr/lib/x86_64-linux-gnu/libstdc++.so.6>

Can you put the code back? If it fails I can help you figure out what the issue is.

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 · 2017-10-17T21:07:29Z

source/common/http/conn_manager_impl.cc

@@ -502,6 +502,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers,
    state_.saw_connection_close_ = true;
  }

+  const HeaderEntry* decorator_operation = request_headers_->EnvoyDecoratorOperation();


So at minimum, I think you are grabbing a header before headers are cleaned, which means this can get deleted. Can you remove the local variable and do this inline below? (Or just pass the full constant header map ref to the trace function).

I'll give it a go tomorrow - although I think the problem is that the x-envoy-decorator-operation is being removed in the sanitize step, as previously when I commented this line out the tests/coverage worked.

Signed-off-by: Gary Brown <gary@brownuk.com>

… internal request Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2017-10-18T10:52:40Z

@mattklein123 Hi Matt, need some help.

https://github.com/envoyproxy/envoy/pull/1858/files#diff-ed63d9053f7d0b3777c96f2aaf8f2f2fR400 - this test is failing when code for sanitizing the header is included. So this suggests this test is representing an external request, even though the comment https://github.com/envoyproxy/envoy/pull/1858/files#diff-ed63d9053f7d0b3777c96f2aaf8f2f2fR409 suggests it is internal.

If this test is handling an external request, then I can change it to correctly check for a null header - and then introduce another test for internal request handling? If this is the case - how should I change the test to represent an internal request?

Other option is to remove the santization code for now and deal with in a separate PR? thoughts?

mattklein123 · 2017-10-18T16:24:08Z

@objectiser I can check it out locally and see, but from a quick glance, given that use_remote_address_ is being set to false, I think you just need to set XFF to an internal address. So in the dispatched headers add something like x-forwarded-for: 10.0.0.1

objectiser · 2017-10-18T17:27:35Z

@mattklein123 Adding that header made the test work - however looks like the sanitization is not having the desired effect. I tried out the jaegertracing example, and the decorated op name is not being propagated from the egress listener to the ingress:

Is it ok to remove the sanitization for now and look at as a separate issue?

mattklein123 · 2017-10-18T17:33:07Z

Is it ok to remove the sanitization for now and look at as a separate issue?

Sure that's fine. Please open an issue so that we can track/remember.

Signed-off-by: Gary Brown <gary@brownuk.com>

…1858) Description: Introduces an initial heuristic for attempting a connection using a socket with a bound interface. The heuristic is entirely contained within reportNetworkUsage(...), and as such is straightforward to iterate and experiment upon. In support of the heuristic, current network/configuration state is identified with a configuration_key that prevents stale information from influencing internal tracking. ~Current network state is stored and updated automatically.~ This allows static accessors to begin to report OS updates during Envoy's startup period, and ensures OS updates have a timely effect on new requests. The initial heuristic is as follows: For both WiFi and cellular network types, there is a default socket configuration that does not use a bound socket, and an alternative "override" configuration that will attempt to use an interface associated with the *opposite* network type (a cellular-bound socket when the preferred network type is WiFi, and a WiFi-bound socket when network type is cellular). A "network fault" is defined as a request that terminates in error while having received no upstream bytes. If a connection has never handled a request without a fault, it is allowed only one fault before the alternative will be tried. If a connection has ever handled a request without a fault, it is allowed up to three consecutive faults before the alternative will be tried. The above behavior is disabled by default, and may be enabled by settings enableInterfaceBinding(true) on the public EngineBuilders. Risk Level: Moderate Testing: Manual and new/updated coverage Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

tracing: Enable decorated operation in outbound (egress) listener to …

60a3aa2

…be passed to inbound (ingress) listener to override server span operation Signed-off-by: Gary Brown <gary@brownuk.com>

Add documentation for the new header

4bb6486

Signed-off-by: Gary Brown <gary@brownuk.com>

mattklein123 reviewed Oct 16, 2017

View reviewed changes

objectiser added 3 commits October 16, 2017 19:53

Fix review comments

2a6d86d

Signed-off-by: Gary Brown <gary@brownuk.com>

Fix use of freed header entry

31f5479

Signed-off-by: Gary Brown <gary@brownuk.com>

Fix coverage failure

c996571

Signed-off-by: Gary Brown <gary@brownuk.com>

rshriram reviewed Oct 17, 2017

View reviewed changes

mattklein123 reviewed Oct 17, 2017

View reviewed changes

objectiser added 2 commits October 17, 2017 21:13

Add fullstops

a73de8c

Signed-off-by: Gary Brown <gary@brownuk.com>

Pass header entry rather than string

40b0b28

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the overrideop branch from 57116af to 40b0b28 Compare October 17, 2017 20:59

mattklein123 reviewed Oct 17, 2017

View reviewed changes

objectiser added 2 commits October 18, 2017 11:24

Remove sanitization of the decorator-operation header for now

d129e89

Signed-off-by: Gary Brown <gary@brownuk.com>

Added sanitization back in, but now fails on test which should be for…

accf425

… internal request Signed-off-by: Gary Brown <gary@brownuk.com>

Remove sanitization of decorator-operation header for now

43be090

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser mentioned this pull request Oct 18, 2017

tracing: Investigate supporting header sanitization for x-envoy-decorator-operation #1889

Closed

mattklein123 approved these changes Oct 18, 2017

View reviewed changes

mattklein123 merged commit fb83d88 into envoyproxy:master Oct 18, 2017

objectiser deleted the overrideop branch October 18, 2017 19:18

This was referenced Oct 18, 2017

Use the route name to define the tracing span/operation name. istio/old_pilot_repo#1406

Merged

tracing: Use openapi definition for service to enhance tracing information #1897

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: Enable decorated operation in outbound (egress) listener to … #1858

tracing: Enable decorated operation in outbound (egress) listener to … #1858

objectiser commented Oct 15, 2017

mattklein123 commented Oct 16, 2017

mattklein123 Oct 16, 2017

mattklein123 Oct 16, 2017

mattklein123 Oct 16, 2017

mattklein123 Oct 16, 2017

mattklein123 Oct 16, 2017

mattklein123 Oct 16, 2017

rshriram Oct 17, 2017

mattklein123 Oct 17, 2017

rshriram left a comment

mattklein123 left a comment

mattklein123 Oct 17, 2017

objectiser Oct 17, 2017

mattklein123 Oct 17, 2017

objectiser Oct 17, 2017

mattklein123 Oct 17, 2017

objectiser Oct 17, 2017

mattklein123 Oct 17, 2017

objectiser Oct 17, 2017

objectiser commented Oct 18, 2017

mattklein123 commented Oct 18, 2017

objectiser commented Oct 18, 2017

mattklein123 commented Oct 18, 2017

		@@ -12,6 +12,7 @@ depends on the :ref:`use_remote_address <config_http_conn_man_use_remote_address

		Envoy will potentially sanitize the following headers:

		* :ref:`x-envoy-decorator-operation <config_http_filters_router_x-envoy-decorator-operation>`


		active_span_ = connection_manager_.tracer_.startSpan(this, request_headers_, request_info_);

		// If a decorator has been defined, apply it to the active span

tracing: Enable decorated operation in outbound (egress) listener to … #1858

tracing: Enable decorated operation in outbound (egress) listener to … #1858

Conversation

objectiser commented Oct 15, 2017

mattklein123 commented Oct 16, 2017

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

rshriram 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

objectiser commented Oct 18, 2017

mattklein123 commented Oct 18, 2017

objectiser commented Oct 18, 2017

mattklein123 commented Oct 18, 2017