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

http conn man: add tracing config for path length in tag #8095

Merged
Merged
Show file tree
Hide file tree
Changes from 5 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
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,11 @@ message HttpConnectionManager {
// Whether to annotate spans with additional data. If true, spans will include logs for stream
// events.
bool verbose = 6;

// Maximum length of the request path to extract and include in the HttpUrl tag. Used to
// truncate lengthy request paths to meet the needs of a tracing backend.
// Default: 256
uint32 max_path_tag_length = 7;
douglas-reid marked this conversation as resolved.
Show resolved Hide resolved
}

// Presence of the object defines whether the connection manager
Expand Down
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ Version history
certificate validation context.
* tracing: added tags for gRPC response status and meesage.
* upstream: added :ref:`an option <envoy_api_field_Cluster.CommonLbConfig.close_connections_on_host_set_change>` that allows draining HTTP, TCP connection pools on cluster membership change.
* tracing: added :ref:`max_path_tag_length <envoy_api_field_config.filter.network.http_connection_manager.v2.HttpConnectionManager.tracing>` to support customizing the length of the request path included in the extracted HttpUrl tag.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you might want to do a :ref: back to HttpUrl message definition as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a pointer to the external definition of the tag. that ok?

* upstream: added network filter chains to upstream connections, see :ref:`filters<envoy_api_field_Cluster.filters>`.
* upstream: use p2c to select hosts for least-requests load balancers if all host weights are the same, even in cases where weights are not equal to 1.
* zookeeper: parse responses and emit latency stats.
Expand Down
5 changes: 5 additions & 0 deletions include/envoy/tracing/http_tracer.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ class Config {
* @return true if spans should be annotated with more detailed information.
*/
virtual bool verbose() const PURE;

/**
* @return the maximum length allowed for paths in the extracted HttpUrl tag.
*/
virtual uint32_t maxPathTagLength() const PURE;
};

class Span;
Expand Down
1 change: 1 addition & 0 deletions source/common/http/conn_manager_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ struct TracingConnectionManagerConfig {
envoy::type::FractionalPercent random_sampling_;
envoy::type::FractionalPercent overall_sampling_;
bool verbose_;
uint32_t max_path_tag_length_;
};

using TracingConnectionManagerConfigPtr = std::unique_ptr<TracingConnectionManagerConfig>;
Expand Down
4 changes: 4 additions & 0 deletions source/common/http/conn_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1757,6 +1757,10 @@ bool ConnectionManagerImpl::ActiveStream::verbose() const {
return connection_manager_.config_.tracingConfig()->verbose_;
}

uint32_t ConnectionManagerImpl::ActiveStream::maxPathTagLength() const {
return connection_manager_.config_.tracingConfig()->max_path_tag_length_;
}

void ConnectionManagerImpl::ActiveStream::callHighWatermarkCallbacks() {
++high_watermark_count_;
for (auto watermark_callbacks : watermark_callbacks_) {
Expand Down
1 change: 1 addition & 0 deletions source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
Tracing::OperationName operationName() const override;
const std::vector<Http::LowerCaseString>& requestHeadersForTags() const override;
bool verbose() const override;
uint32_t maxPathTagLength() const override;

// ScopeTrackedObject
void dumpState(std::ostream& os, int indent_level = 0) const override {
Expand Down
8 changes: 5 additions & 3 deletions source/common/tracing/http_tracer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@ static std::string valueOrDefault(const Http::HeaderEntry* header, const char* d
return header ? std::string(header->value().getStringView()) : default_value;
}

static std::string buildUrl(const Http::HeaderMap& request_headers) {
static std::string buildUrl(const Http::HeaderMap& request_headers,
const uint32_t max_path_length) {
std::string path(request_headers.EnvoyOriginalPath()
? request_headers.EnvoyOriginalPath()->value().getStringView()
: request_headers.Path()->value().getStringView());
static const size_t max_path_length = 256;

if (path.length() > max_path_length) {
path = path.substr(0, max_path_length);
}
Expand Down Expand Up @@ -148,7 +149,8 @@ void HttpTracerUtility::finalizeSpan(Span& span, const Http::HeaderMap* request_
span.setTag(Tracing::Tags::get().GuidXRequestId,
std::string(request_headers->RequestId()->value().getStringView()));
}
span.setTag(Tracing::Tags::get().HttpUrl, buildUrl(*request_headers));
span.setTag(Tracing::Tags::get().HttpUrl,
buildUrl(*request_headers, tracing_config.maxPathTagLength()));
span.setTag(Tracing::Tags::get().HttpMethod,
std::string(request_headers->Method()->value().getStringView()));
span.setTag(Tracing::Tags::get().DownstreamCluster,
Expand Down
1 change: 1 addition & 0 deletions source/common/tracing/http_tracer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class EgressConfigImpl : public Config {
return request_headers_for_tags_;
}
bool verbose() const override { return false; }
uint32_t maxPathTagLength() const override { return 256; }
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid hardcoding 256 in places? Suggest a shared constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasn't quite sure where to put it, think I found a reasonable spot. updated useful places to use that constant.


private:
const std::vector<Http::LowerCaseString> request_headers_for_tags_{};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,13 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
overall_sampling.set_numerator(
tracing_config.has_overall_sampling() ? tracing_config.overall_sampling().value() : 100);

uint32_t max_path_tag_length =
tracing_config.max_path_tag_length() > 0 ? tracing_config.max_path_tag_length() : 256;

tracing_config_ =
std::make_unique<Http::TracingConnectionManagerConfig>(Http::TracingConnectionManagerConfig{
tracing_operation_name, request_headers_for_tags, client_sampling, random_sampling,
overall_sampling, tracing_config.verbose()});
overall_sampling, tracing_config.verbose(), max_path_tag_length});
}

for (const auto& access_log : config.access_log()) {
Expand Down
12 changes: 8 additions & 4 deletions test/common/http/conn_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan
percent1,
percent2,
percent1,
false});
false,
256});
}
}

Expand Down Expand Up @@ -984,7 +985,8 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato
percent1,
percent2,
percent1,
false});
false,
256});

auto* span = new NiceMock<Tracing::MockSpan>();
EXPECT_CALL(tracer_, startSpan_(_, _, _, _))
Expand Down Expand Up @@ -1062,7 +1064,8 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato
percent1,
percent2,
percent1,
false});
false,
256});

auto* span = new NiceMock<Tracing::MockSpan>();
EXPECT_CALL(tracer_, startSpan_(_, _, _, _))
Expand Down Expand Up @@ -1135,7 +1138,8 @@ TEST_F(HttpConnectionManagerImplTest,
percent1,
percent2,
percent1,
false});
false,
256});

EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled",
An<const envoy::type::FractionalPercent&>(), _))
Expand Down
3 changes: 2 additions & 1 deletion test/common/http/conn_manager_utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ class ConnectionManagerUtilityTest : public testing::Test {
envoy::type::FractionalPercent percent2;
percent2.set_numerator(10000);
percent2.set_denominator(envoy::type::FractionalPercent::TEN_THOUSAND);
tracing_config_ = {Tracing::OperationName::Ingress, {}, percent1, percent2, percent1, false};
tracing_config_ = {
Tracing::OperationName::Ingress, {}, percent1, percent2, percent1, false, 256};
ON_CALL(config_, tracingConfig()).WillByDefault(Return(&tracing_config_));

ON_CALL(config_, via()).WillByDefault(ReturnRef(via_));
Expand Down
1 change: 1 addition & 0 deletions test/common/tracing/http_tracer_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,7 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) {
EXPECT_CALL(span, setTag(Eq("cc"), Eq("c")));
EXPECT_CALL(config, requestHeadersForTags());
EXPECT_CALL(config, verbose).WillOnce(Return(false));
EXPECT_CALL(config, maxPathTagLength).WillOnce(Return(256));
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth setting the length to something lower and check that the http.url tag actually has a truncated value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is another test for that in this same file. OriginalAndLongPath uses a 300 character path and an expected path of 256 in length.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that is fine.


absl::optional<uint32_t> response_code(503);
EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ stat_prefix: router
operation_name: ingress
request_headers_for_tags:
- foo
max_path_tag_length: 128
http_filters:
- name: envoy.router
config: {}
Expand All @@ -235,6 +236,7 @@ stat_prefix: router

EXPECT_THAT(std::vector<Http::LowerCaseString>({Http::LowerCaseString("foo")}),
ContainerEq(config.tracingConfig()->request_headers_for_tags_));
EXPECT_EQ(128, config.tracingConfig()->max_path_tag_length_);
EXPECT_EQ(*context_.local_info_.address_, config.localAddress());
EXPECT_EQ("foo", config.serverName());
EXPECT_EQ(HttpConnectionManagerConfig::HttpConnectionManagerProto::OVERWRITE,
Expand All @@ -260,6 +262,7 @@ TEST_F(HttpConnectionManagerConfigTest, SamplingDefault) {
scoped_routes_config_provider_manager_);

EXPECT_EQ(100, config.tracingConfig()->client_sampling_.numerator());
EXPECT_EQ(256, config.tracingConfig()->max_path_tag_length_);
EXPECT_EQ(envoy::type::FractionalPercent::HUNDRED,
config.tracingConfig()->client_sampling_.denominator());
EXPECT_EQ(10000, config.tracingConfig()->random_sampling_.numerator());
Expand Down
1 change: 1 addition & 0 deletions test/mocks/tracing/mocks.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ MockConfig::MockConfig() {
ON_CALL(*this, operationName()).WillByDefault(Return(operation_name_));
ON_CALL(*this, requestHeadersForTags()).WillByDefault(ReturnRef(headers_));
ON_CALL(*this, verbose()).WillByDefault(Return(verbose_));
ON_CALL(*this, maxPathTagLength()).WillByDefault(Return(uint32_t(256)));
}
MockConfig::~MockConfig() = default;

Expand Down
1 change: 1 addition & 0 deletions test/mocks/tracing/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ class MockConfig : public Config {
MOCK_CONST_METHOD0(operationName, OperationName());
MOCK_CONST_METHOD0(requestHeadersForTags, const std::vector<Http::LowerCaseString>&());
MOCK_CONST_METHOD0(verbose, bool());
MOCK_CONST_METHOD0(maxPathTagLength, uint32_t());

OperationName operation_name_{OperationName::Ingress};
std::vector<Http::LowerCaseString> headers_;
Expand Down