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

Jump to bottom

Return x-request-id when tracing enforced. #27

Merged
merged 9 commits into from
Aug 22, 2016
Merged

Return x-request-id when tracing enforced. #27

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1027357
x-request-id preparation work
RomanDzhabarov Aug 19, 2016
574acca
Add tests for isForcedTracing.
RomanDzhabarov Aug 21, 2016
e899995
Move logic into mutateResponseHeaders.
RomanDzhabarov Aug 21, 2016
31a4b25
revert source/common/http/conn_manager_impl.h.
RomanDzhabarov Aug 21, 2016
98d4672
Update headers.rst
mattklein123 Aug 21, 2016
ccfe069
Clean x-envoy-force-trace header on not internal requests.
RomanDzhabarov Aug 21, 2016
7c5ef2b
Merge branch 'return-rq' of github.com:lyft/envoy into return-rq
RomanDzhabarov Aug 21, 2016
ac25700
revert source/common/tracing/http_tracer_impl.h
RomanDzhabarov Aug 21, 2016
e9ca23a
s/ForceTrace/EnvoyForceTrace.
RomanDzhabarov Aug 22, 2016
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
12 changes: 7 additions & 5 deletions docs/configuration/http_conn_man/headers.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,13 @@ x-envoy-force-trace
-------------------

If an internal request sets this header, Envoy will modify the generated
:ref:`config_http_conn_man_headers_x-request-id` such that it forces traces to be collected. If this
request ID is then propagated to other hosts, traces will also be collected on those hosts which
will provide a consistent trace for an entire request flow. See the :ref:`tracing.global_enabled
<config_http_conn_man_runtime_global_enabled>` and :ref:`tracing.random_sampling
<config_http_conn_man_runtime_random_sampling>` runtime configuration settings.
:ref:`config_http_conn_man_headers_x-request-id` such that it forces traces to be collected.
This also forces :ref:`config_http_conn_man_headers_x-request-id` to be returned in the response
headers. If this request ID is then propagated to other hosts, traces will also be collected on
those hosts which will provide a consistent trace for an entire request flow. See the
:ref:`tracing.global_enabled <config_http_conn_man_runtime_global_enabled>` and
:ref:`tracing.random_sampling <config_http_conn_man_runtime_random_sampling>` runtime
configuration settings.

.. _config_http_conn_man_headers_x-envoy-internal:

Expand Down
3 changes: 2 additions & 1 deletion source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,8 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
headers.replaceViaMoveValue(Headers::get().Date, date_formatter_.now());
headers.replaceViaCopy(Headers::get().EnvoyProtocolVersion,
connection_manager_.codec_->protocolString());
ConnectionManagerUtility::mutateResponseHeaders(headers, connection_manager_.config_);
ConnectionManagerUtility::mutateResponseHeaders(headers, *request_headers_,
connection_manager_.config_);

// See if we want to drain/close the connection. Send the go away frame prior to encoding the
// header block.
Expand Down
7 changes: 7 additions & 0 deletions source/common/http/conn_manager_utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "common/http/utility.h"
#include "common/network/utility.h"
#include "common/runtime/uuid_util.h"
#include "common/tracing/http_tracer_impl.h"

namespace Http {

Expand Down Expand Up @@ -116,6 +117,7 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea
}

void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_headers,
const Http::HeaderMap& request_headers,
ConnectionManagerConfig& config) {
response_headers.remove(Headers::get().Connection);
response_headers.remove(Headers::get().TransferEncoding);
Expand All @@ -129,6 +131,11 @@ void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_h
config.routeConfig().responseHeadersToAdd()) {
response_headers.addViaCopy(to_add.first, to_add.second);
}

if (Tracing::HttpTracerUtility::isForcedTracing(request_headers)) {
response_headers.replaceViaCopy(Headers::get().RequestId,
request_headers.get(Headers::get().RequestId));
}
}

} // Http
1 change: 1 addition & 0 deletions source/common/http/conn_manager_utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class ConnectionManagerUtility {
Runtime::RandomGenerator& random, Runtime::Loader& runtime);

static void mutateResponseHeaders(Http::HeaderMap& response_headers,
const Http::HeaderMap& request_headers,
ConnectionManagerConfig& config);
};

Expand Down
5 changes: 5 additions & 0 deletions source/common/tracing/http_tracer_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques
return {Reason::NotTraceableRequestId, false};
}

bool HttpTracerUtility::isForcedTracing(const Http::HeaderMap& request_headers) {
return request_headers.has(Http::Headers::get().EnvoyInternalRequest) &&
request_headers.has(Http::Headers::get().ForceTrace);
Copy link
Member

Choose a reason for hiding this comment

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

It's actually a bug that ForceTrace is not cleaned from external headers. You should clean it (and test for it) and the mutate request headers section in ConnManagerUtility. Then this check only needs to look for ForceTrace. IMO ForceTrace should also be cleaned entirely once it's processed (whether internal or external), to avoid propagating to the next hop, but that's a larger change.

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 it (cleaning it on non internal requests).

although propagating is possible this will require service to read/copy specific header and pass it to another egress call.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I meant that right now if someone sets force-trace, we still send that header on, which will trigger force trace on the next hop. That hop may or may not have tracing enabled, and it doesn't really matter since we have already modified x-request-id, but technically I think if someone sets force trace header we should clean it no matter what after it is used. This is not a big deal.

}

HttpTracerImpl::HttpTracerImpl(Runtime::Loader& runtime, Stats::Store& stats)
: runtime_(runtime),
stats_{HTTP_TRACER_STATS(POOL_COUNTER_PREFIX(stats, "tracing.http_tracer."))} {}
Expand Down
5 changes: 5 additions & 0 deletions source/common/tracing/http_tracer_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ class HttpTracerUtility {
**/
static Decision isTracing(const Http::AccessLog::RequestInfo& request_info,
const Http::HeaderMap& request_headers, Runtime::Loader& runtime);

/**
* @return true if request is forced by service and it's internal request.
*/
static bool isForcedTracing(const Http::HeaderMap& request_headers);
};

class HttpTracerImpl : public HttpTracer {
Expand Down
27 changes: 20 additions & 7 deletions test/common/http/conn_manager_utility_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,14 +312,27 @@ TEST_F(ConnectionManagerUtilityTest, MutateResponseHeaders) {
config_.route_config_.response_headers_to_remove_.push_back("custom_header");
config_.route_config_.response_headers_to_add_.push_back({"to_add", "foo"});

HeaderMapImpl headers{{"connection", "foo"},
{"transfer-encoding", "foo"},
{":version", "foo"},
{"custom_header", "foo"}};
ConnectionManagerUtility::mutateResponseHeaders(headers, config_);
HeaderMapImpl response_headers{{"connection", "foo"},
{"transfer-encoding", "foo"},
{":version", "foo"},
{"custom_header", "foo"}};
HeaderMapImpl request_headers{{"x-request-id", "request-id"}};

ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, config_);

EXPECT_EQ(1UL, response_headers.size());
EXPECT_EQ("foo", response_headers.get("to_add"));
EXPECT_FALSE(response_headers.has("x-request-id"));
}

TEST_F(ConnectionManagerUtilityTest, MutateResponseHeadersReturnXRequestId) {
HeaderMapImpl response_headers;
HeaderMapImpl request_headers{{"x-request-id", "request-id"},
{"x-envoy-internal", "true"},
{"x-envoy-force-trace", "true"}};

EXPECT_EQ(1UL, headers.size());
EXPECT_EQ("foo", headers.get("to_add"));
ConnectionManagerUtility::mutateResponseHeaders(response_headers, request_headers, config_);
EXPECT_EQ("request-id", response_headers.get("x-request-id"));
}

} // Http
11 changes: 11 additions & 0 deletions test/common/tracing/http_tracer_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@ using testing::Test;

namespace Tracing {

TEST(HttpTracerUtilityTest, isForcedTracing) {
Http::HeaderMapImpl forced_header{{"x-envoy-internal", "true"}, {"x-envoy-force-trace", "true"}};
EXPECT_TRUE(HttpTracerUtility::isForcedTracing(forced_header));

Http::HeaderMapImpl not_internal{{"x-envoy-force-trace", "true"}};
EXPECT_FALSE(HttpTracerUtility::isForcedTracing(not_internal));

Http::HeaderMapImpl not_forced{{"x-envoy-internal", "true"}};
EXPECT_FALSE(HttpTracerUtility::isForcedTracing(not_forced));
}

TEST(HttpTracerUtilityTest, IsTracing) {
NiceMock<Http::AccessLog::MockRequestInfo> request_info;
NiceMock<Runtime::MockLoader> runtime;
Expand Down