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.

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

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

Merged
merged 9 commits into from
Aug 22, 2016
12 changes: 7 additions & 5 deletions docs/configuration/http_conn_man/headers.rst
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
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
12 changes: 10 additions & 2 deletions source/common/http/conn_manager_utility.cc
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 @@ -65,6 +66,7 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea
request_headers.remove(Headers::get().EnvoyUpstreamRequestTimeoutMs);
request_headers.remove(Headers::get().EnvoyUpstreamRequestPerTryTimeoutMs);
request_headers.remove(Headers::get().EnvoyExpectedRequestTimeoutMs);
request_headers.remove(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.

nit: can you rename this "EnvoyForceTrace" (and reorder in headers.h). Not related to this review but might as well fix now. Otherwise looks good.


for (const Http::LowerCaseString& header : config.routeConfig().internalOnlyHeaders()) {
request_headers.remove(header);
Expand Down Expand Up @@ -107,15 +109,16 @@ void ConnectionManagerUtility::mutateRequestHeaders(Http::HeaderMap& request_hea
}
}

// Make request traceable if it's internal and ForceTrace header is set.
if (internal_request && request_headers.has(Headers::get().ForceTrace)) {
// Make request traceable if x-envoy-force-trace header is set.
if (request_headers.has(Headers::get().ForceTrace)) {
std::string uuid = request_headers.get(Headers::get().RequestId);
UuidUtils::setTraceableUuid(uuid);
request_headers.replaceViaMoveValue(Headers::get().RequestId, std::move(uuid));
}
}

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 +132,11 @@ void ConnectionManagerUtility::mutateResponseHeaders(Http::HeaderMap& response_h
config.routeConfig().responseHeadersToAdd()) {
response_headers.addViaCopy(to_add.first, to_add.second);
}

if (request_headers.has(Headers::get().ForceTrace)) {
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
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
28 changes: 20 additions & 8 deletions test/common/http/conn_manager_utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,14 @@ TEST_F(ConnectionManagerUtilityTest, InternalServiceForceTrace) {
}

{
// Not internal request, force trace should be ignored
// Not internal request, force trace header should be cleaned.
HeaderMapImpl headers{{"x-forwarded-for", external_remote_address},
{"x-request-id", uuid},
{"x-envoy-force-trace", "true"}};
ConnectionManagerUtility::mutateRequestHeaders(headers, connection_, config_, random_,
runtime_);
EXPECT_EQ(uuid, headers.get(Headers::get().RequestId));
EXPECT_FALSE(headers.has(Headers::get().ForceTrace));
}
}

Expand Down Expand Up @@ -312,14 +313,25 @@ 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-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