Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -128,7 +128,7 @@ message HttpConnectionManager {
UNESCAPE_AND_FORWARD = 4;
}

// [#next-free-field: 10]
// [#next-free-field: 11]
message Tracing {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager.Tracing";
Expand Down Expand Up @@ -193,6 +193,11 @@ message HttpConnectionManager {
// Such a constraint is inherent to OpenCensus itself. It cannot be overcome without changes
// on OpenCensus side.
config.trace.v3.Tracing.Http provider = 9;

// Trace reason is decided by request_id by default. If true, these sampling strategy decisions
// will be decided by only tracing provider and :ref:`x-request-id<config_http_conn_man_headers_x-request-id>`
// never be modified.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure if I understand this correctly, but please let me know if I'm wrong here:

Suggested change
// Trace reason is decided by request_id by default. If true, these sampling strategy decisions
// will be decided by only tracing provider and :ref:`x-request-id<config_http_conn_man_headers_x-request-id>`
// never be modified.
// By default, tracing requires the :ref:`x-request-id <config_http_conn_man_headers_x-request-id>` header to be set, and modifies its value.
// If true, the request will be traced, and *x-request-id* will not be modified.

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.

I think that we should add that the request will bypass x-request-id based tracing decision.

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 this be an enum? Do we want to maybe support other modes of operation in the future?

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.

What operation will be expected in the future?

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 was interested in your perspective on this.

@Shikugawa Shikugawa Jun 14, 2021

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.

@htuch It can be changed as follows.

enum TraceRequestIdSampleDecision {
  None,
  Bypass, // It acts as `bypass_sampling_with_request_id` is true.
}

WDYT? cc. @lizan

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.

Sure, with NONE, BYPASS. If you have high confidence we would never have some other trace sample strategy with request ID I'm fine with the current bool, just would like to make sure this is deliberative.

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.

done

bool bypass_sampling_with_request_id = 10;
}

message InternalAddressConfig {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ Minor Behavior Changes
* listener: respect the :ref:`connection balance config <envoy_v3_api_field_config.listener.v3.Listener.connection_balance_config>`
defined within the listener where the sockets are redirected to. Clear that field to restore the previous behavior.
* tcp: switched to the new connection pool by default. Any unexpected behavioral changes can be reverted by setting runtime guard ``envoy.reloadable_features.new_tcp_connection_pool`` to false.
* tracing: add :ref:`bypass_sampling_with_request_id <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.Tracing.bypass_sampling_with_request_id>` to bypass current :ref:`x-request-id<config_http_conn_man_headers_x-request-id>` based sampling decision behavior.

Bug Fixes
---------
Expand Down
5 changes: 5 additions & 0 deletions envoy/tracing/trace_driver.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ class Config {
* for HTTP protocol tracing.
*/
virtual uint32_t maxPathTagLength() const PURE;

/**
* @return true if trace reason won't be decided by request_id value.
*/
virtual bool bypassSamplingWithRequestId() const PURE;
};

/**
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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 @@ -132,6 +132,7 @@ struct TracingConnectionManagerConfig {
envoy::type::v3::FractionalPercent overall_sampling_;
bool verbose_;
uint32_t max_path_tag_length_;
bool bypass_sampling_with_request_id_;
};

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 @@ -1585,6 +1585,10 @@ uint32_t ConnectionManagerImpl::ActiveStream::maxPathTagLength() const {
return connection_manager_.config_.tracingConfig()->max_path_tag_length_;
}

bool ConnectionManagerImpl::ActiveStream::bypassSamplingWithRequestId() const {
return connection_manager_.config_.tracingConfig()->bypass_sampling_with_request_id_;
}

const Router::RouteEntry::UpgradeMap* ConnectionManagerImpl::ActiveStream::upgradeMap() {
// We must check if the 'cached_route_' optional is populated since this function can be called
// early via sendLocalReply(), before the cached route is populated.
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 @@ -201,6 +201,7 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
const Tracing::CustomTagMap* customTags() const override;
bool verbose() const override;
uint32_t maxPathTagLength() const override;
bool bypassSamplingWithRequestId() const override;

// ScopeTrackedObject
void dumpState(std::ostream& os, int indent_level = 0) const override {
Expand Down
3 changes: 3 additions & 0 deletions source/common/http/conn_manager_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,9 @@ Tracing::Reason ConnectionManagerUtility::mutateTracingRequestHeader(
if (!config.tracingConfig()) {
return final_reason;
}
if (config.tracingConfig()->bypass_sampling_with_request_id_) {
return Tracing::Reason::Sampling;
}

auto rid_extension = config.requestIDExtension();
const auto rid_to_integer = rid_extension->toInteger(request_headers);
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 @@ -81,6 +81,7 @@ class EgressConfigImpl : public Config {
const CustomTagMap* customTags() const override { return nullptr; }
bool verbose() const override { return false; }
uint32_t maxPathTagLength() const override { return Tracing::DefaultMaxPathTagLength; }
bool bypassSamplingWithRequestId() const override { return false; }
};

using EgressConfig = ConstSingleton<EgressConfigImpl>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
tracing_config_ =
std::make_unique<Http::TracingConnectionManagerConfig>(Http::TracingConnectionManagerConfig{
tracing_operation_name, custom_tags, client_sampling, random_sampling, overall_sampling,
tracing_config.verbose(), max_path_tag_length});
tracing_config.verbose(), max_path_tag_length,
tracing_config.bypass_sampling_with_request_id()});
}

for (const auto& access_log : config.access_log()) {
Expand Down
14 changes: 9 additions & 5 deletions test/common/http/conn_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1374,7 +1374,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) {
}
tracing_config_ = std::make_unique<TracingConnectionManagerConfig>(
TracingConnectionManagerConfig{Tracing::OperationName::Ingress, conn_tracing_tags, percent1,
percent2, percent1, false, 256});
percent2, percent1, false, 256, false});
NiceMock<Router::MockRouteTracing> route_tracing;
ON_CALL(route_tracing, getClientSampling()).WillByDefault(ReturnRef(percent1));
ON_CALL(route_tracing, getRandomSampling()).WillByDefault(ReturnRef(percent2));
Expand Down Expand Up @@ -1656,7 +1656,8 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato
percent2,
percent1,
false,
256});
256,
false});

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

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

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

EXPECT_CALL(
runtime_.snapshot_,
Expand Down
3 changes: 2 additions & 1 deletion test/common/http/conn_manager_impl_test_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ void HttpConnectionManagerImplTest::setup(bool ssl, const std::string& server_na
percent2,
percent1,
false,
256});
256,
false});
}
}

Expand Down
11 changes: 10 additions & 1 deletion test/common/http/conn_manager_utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class ConnectionManagerUtilityTest : public testing::Test {
percent2.set_numerator(10000);
percent2.set_denominator(envoy::type::v3::FractionalPercent::TEN_THOUSAND);
tracing_config_ = {
Tracing::OperationName::Ingress, {}, percent1, percent2, percent1, false, 256};
Tracing::OperationName::Ingress, {}, percent1, percent2, percent1, false, 256, false};
ON_CALL(config_, tracingConfig()).WillByDefault(Return(&tracing_config_));
ON_CALL(config_, localReply()).WillByDefault(ReturnRef(*local_reply_));

Expand Down Expand Up @@ -1229,6 +1229,15 @@ TEST_F(ConnectionManagerUtilityTest, SamplingWithoutRouteOverride) {
EXPECT_EQ(Tracing::Reason::Sampling, request_id_extension_->getTraceReason(request_headers));
}

TEST_F(ConnectionManagerUtilityTest, CheckSamplingDecisionWithBypassSamplingWithRequestId) {
tracing_config_.bypass_sampling_with_request_id_ = true;

Http::TestRequestHeaderMapImpl request_headers{
{"x-request-id", "125a4afb-6f55-44ba-ad80-413f09f48a28"}};
const auto ret = callMutateRequestHeaders(request_headers, Protocol::Http2);
EXPECT_EQ(Tracing::Reason::Sampling, ret.trace_reason_);
}

TEST_F(ConnectionManagerUtilityTest, SamplingWithRouteOverride) {
EXPECT_CALL(
runtime_.snapshot_,
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, customTags()).WillByDefault(Return(&custom_tags_));
ON_CALL(*this, verbose()).WillByDefault(Return(verbose_));
ON_CALL(*this, maxPathTagLength()).WillByDefault(Return(uint32_t(256)));
ON_CALL(*this, bypassSamplingWithRequestId()).WillByDefault(Return(false));
}
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 @@ -20,6 +20,7 @@ class MockConfig : public Config {
MOCK_METHOD(const CustomTagMap*, customTags, (), (const));
MOCK_METHOD(bool, verbose, (), (const));
MOCK_METHOD(uint32_t, maxPathTagLength, (), (const));
MOCK_METHOD(bool, bypassSamplingWithRequestId, (), (const));

OperationName operation_name_{OperationName::Ingress};
CustomTagMap custom_tags_;
Expand Down