Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
8 changes: 7 additions & 1 deletion docs/configuration/http_conn_man/tracing.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,18 @@ Tracing

{
"tracing": {
"operation_name": "..."
"operation_name": "...",
"request_headers_for_tags": []
}
}

operation_name
*(required, string)* Span name will be derived from operation_name. "ingress" and "egress"
are the only supported values.

request_headers_for_tags
*(optional, array)* An optional list of header names which is used for populating tags on the an active span.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo

Each tag name is the header name and tag value is the header value from the request headers.
If specified header is not present in the request headers no tag is created.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

grammar



4 changes: 4 additions & 0 deletions include/envoy/http/header_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ class LowerCaseString {

const std::string& get() const { return string_; }
bool operator==(const LowerCaseString& rhs) const { return string_ == rhs.string_; }
LowerCaseString& operator=(const LowerCaseString& rhs) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should not be needed and is probably indicative of doing something wrong.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

that's because Optional does not have a support for moveable types. Would you suggest to add it there?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know what the actual problem you are trying to solve is, but I think you shouldn't need any of it, so unclear on the actual problem.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

as far as i can tell the following happens:
Optional.value(arg) method does copy assignment for the argument passed in, what ends up happening vector is copy assigned and consequently LowerCaseString is copy assigned which is not permitted as we have move ctor for the LowerCaseString and compiler removes implicit operator=(const LowerCaseString&).

That's why one solution is to explicitly define copy assignment.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

per offline convo make config a pointer to a struct and remove operator= from here (so we don't introduce accidental copies).

string_ = rhs.string_;
return *this;
}

private:
void lower() { std::transform(string_.begin(), string_.end(), string_.begin(), tolower); }
Expand Down
8 changes: 8 additions & 0 deletions include/envoy/tracing/http_tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,15 @@ class Config {
public:
virtual ~Config() {}

/**
* Operation name for tracing, e.g., ingress.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

*/
virtual OperationName operationName() const PURE;

/**
* List of headers to populate tags on the active span.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

*/
virtual const std::list<Http::LowerCaseString>& requestHeadersForTags() const PURE;
};

/*
Expand Down
7 changes: 7 additions & 0 deletions source/common/http/conn_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,8 @@ ConnectionManagerImpl::ActiveStream::~ActiveStream() {
if (request_info_.healthCheck()) {
connection_manager_.config_.tracingStats().health_check_.inc();
} else {
Tracing::HttpTracerUtility::populateTagsBasedOnHeaders(*active_span_, *request_headers_,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just move this logic into finalizeSpan

*this);
Tracing::HttpTracerUtility::finalizeSpan(*active_span_, *request_headers_, request_info_);
}
}
Expand Down Expand Up @@ -748,6 +750,11 @@ Tracing::OperationName ConnectionManagerImpl::ActiveStream::operationName() cons
return connection_manager_.config_.tracingConfig().value().operation_name_;
}

const std::list<Http::LowerCaseString>&
ConnectionManagerImpl::ActiveStream::requestHeadersForTags() const {
return connection_manager_.config_.tracingConfig().value().request_headers_for_tags_;
}

void ConnectionManagerImpl::ActiveStreamFilterBase::addResetStreamCallback(
std::function<void()> callback) {
parent_.reset_callbacks_.push_back(callback);
Expand Down
2 changes: 2 additions & 0 deletions source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ struct ConnectionManagerTracingStats {
*/
struct TracingConnectionManagerConfig {
Tracing::OperationName operation_name_;
std::list<Http::LowerCaseString> request_headers_for_tags_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

std::vector

};

/**
Expand Down Expand Up @@ -384,6 +385,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,

// Tracing::TracingConfig
virtual Tracing::OperationName operationName() const override;
virtual const std::list<Http::LowerCaseString>& requestHeadersForTags() const override;

// All state for the stream. Put here for readability. We could move this to a bit field
// eventually if we want.
Expand Down
5 changes: 5 additions & 0 deletions source/common/json/config_schemas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,11 @@ const std::string Json::Schema::HTTP_CONN_NETWORK_FILTER_SCHEMA(R"EOF(
"operation_name" : {
"type" : "string",
"enum": ["ingress", "egress"]
},
"request_headers_for_tags": {
"type" : "array",
"uniqueItems": "true",
"items" : {"type" : "string"}
}
},
"required" : ["operation_name"],
Expand Down
13 changes: 12 additions & 1 deletion source/common/tracing/http_tracer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ Decision HttpTracerUtility::isTracing(const Http::AccessLog::RequestInfo& reques
return {Reason::NotTraceableRequestId, false};
}

throw std::invalid_argument("Unknown trace_status");
NOT_REACHED;
}

void HttpTracerUtility::finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers,
Expand Down Expand Up @@ -145,6 +145,17 @@ void HttpTracerUtility::finalizeSpan(Span& active_span, const Http::HeaderMap& r
active_span.finishSpan();
}

void HttpTracerUtility::populateTagsBasedOnHeaders(Span& active_span,
const Http::HeaderMap& request_headers,
const Config& tracing_config) {
for (const Http::LowerCaseString& header : tracing_config.requestHeadersForTags()) {
const Http::HeaderEntry* entry = request_headers.get(header);
if (entry) {
active_span.setTag(header.get(), entry->value().c_str());
}
}
}

HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& local_info)
: driver_(std::move(driver)), local_info_(local_info) {}

Expand Down
9 changes: 9 additions & 0 deletions source/common/tracing/http_tracer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,15 @@ class HttpTracerUtility {
static void finalizeSpan(Span& active_span, const Http::HeaderMap& request_headers,
const Http::AccessLog::RequestInfo& request_info);

/**
* Populate the active span's tags from request headers based on the list of headers to use.
* @param active_span span to populate.
* @param request_headers request headers.
* @param headers list of headers to be used as tags.
*/
static void populateTagsBasedOnHeaders(Span& active_span, const Http::HeaderMap& request_headers,
const Config& tracing_config);

static const std::string INGRESS_OPERATION;
static const std::string EGRESS_OPERATION;
};
Expand Down
19 changes: 16 additions & 3 deletions source/server/config/network/http_connection_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,26 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(const Json::Object& con
}

if (config.hasObject("tracing")) {
const std::string operation_name = config.getObject("tracing")->getString("operation_name");
Json::ObjectPtr tracing_config = config.getObject("tracing");

const std::string operation_name = tracing_config->getString("operation_name");
Tracing::OperationName tracing_operation_name;
std::list<Http::LowerCaseString> request_headers_for_tags;

if (operation_name == "ingress") {
tracing_config_.value({Tracing::OperationName::Ingress});
tracing_operation_name = Tracing::OperationName::Ingress;
} else {
ASSERT(operation_name == "egress");
tracing_config_.value({Tracing::OperationName::Egress});
tracing_operation_name = Tracing::OperationName::Egress;
}

if (tracing_config->hasObject("request_headers_for_tags")) {
for (const std::string& header : tracing_config->getStringArray("request_headers_for_tags")) {
request_headers_for_tags.push_back(Http::LowerCaseString(header));
}
}

tracing_config_.value({tracing_operation_name, request_headers_for_tags});
}

if (config.hasObject("idle_timeout_s")) {
Expand Down
5 changes: 4 additions & 1 deletion test/common/http/conn_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class HttpConnectionManagerImplTest : public Test, public ConnectionManagerConfi
"",
fake_stats_},
tracing_stats_{CONN_MAN_TRACING_STATS(POOL_COUNTER(fake_stats_))} {
tracing_config_.value({Tracing::OperationName::Ingress});
tracing_config_.value({Tracing::OperationName::Ingress, {Http::LowerCaseString(":method")}});
}

~HttpConnectionManagerImplTest() {
Expand Down Expand Up @@ -221,6 +221,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) {
return span;
}));
EXPECT_CALL(*span, finishSpan());
EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber());
// Verify tag is set based on the request headers.
EXPECT_CALL(*span, setTag(":method", "GET"));
EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
.WillOnce(Return(true));

Expand Down
2 changes: 1 addition & 1 deletion test/common/http/conn_manager_utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class ConnectionManagerUtilityTest : public testing::Test {
ConnectionManagerUtilityTest() {
ON_CALL(config_, userAgent()).WillByDefault(ReturnRef(user_agent_));

tracing_config_.value({Tracing::OperationName::Ingress});
tracing_config_.value({Tracing::OperationName::Ingress, {}});
ON_CALL(config_, tracingConfig()).WillByDefault(ReturnRef(tracing_config_));
}

Expand Down
16 changes: 16 additions & 0 deletions test/common/tracing/http_tracer_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,22 @@ TEST(HttpTracerUtilityTest, SpanPopulatedFailureResponse) {
HttpTracerUtility::finalizeSpan(*span, request_headers, request_info);
}

TEST(HttpTracerUtilityTest, SpanPopulatedFromRequestHeaders) {
std::unique_ptr<NiceMock<MockSpan>> span(new NiceMock<MockSpan>());
Http::TestHeaderMapImpl request_headers{{"aa", "a"}, {"bb", "b"}, {"cc", "c"}, {"dd", "d"}};

MockConfig config;
config.headers_.push_back(Http::LowerCaseString("aa"));
config.headers_.push_back(Http::LowerCaseString("cc"));
config.headers_.push_back(Http::LowerCaseString("ee"));

EXPECT_CALL(*span, setTag("aa", "a"));
EXPECT_CALL(*span, setTag("cc", "c"));
EXPECT_CALL(config, requestHeadersForTags());

HttpTracerUtility::populateTagsBasedOnHeaders(*span, request_headers, config);
}

TEST(HttpTracerUtilityTest, operationTypeToString) {
EXPECT_EQ("ingress", HttpTracerUtility::toString(OperationName::Ingress));
EXPECT_EQ("egress", HttpTracerUtility::toString(OperationName::Egress));
Expand Down
6 changes: 5 additions & 1 deletion test/mocks/tracing/mocks.cc
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
#include "mocks.h"

using testing::Return;
using testing::ReturnRef;

namespace Tracing {

MockSpan::MockSpan() {}
MockSpan::~MockSpan() {}

MockConfig::MockConfig() { ON_CALL(*this, operationName()).WillByDefault(Return(operation_name_)); }
MockConfig::MockConfig() {
ON_CALL(*this, operationName()).WillByDefault(Return(operation_name_));
ON_CALL(*this, requestHeadersForTags()).WillByDefault(ReturnRef(headers_));
}
MockConfig::~MockConfig() {}

MockHttpTracer::MockHttpTracer() {}
Expand Down
2 changes: 2 additions & 0 deletions test/mocks/tracing/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ class MockConfig : public Config {
~MockConfig();

MOCK_CONST_METHOD0(operationName, OperationName());
MOCK_CONST_METHOD0(requestHeadersForTags, const std::list<Http::LowerCaseString>&());

OperationName operation_name_{OperationName::Ingress};
std::list<Http::LowerCaseString> headers_;
};

class MockSpan : public Span {
Expand Down