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

api: use traffic_direction over operation_name if specified #7999

Merged
merged 8 commits into from
Sep 4, 2019
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -79,7 +79,6 @@ message HttpConnectionManager {
google.protobuf.BoolValue add_user_agent = 6;

message Tracing {
// [#comment:TODO(kyessenov): Align this field with listener traffic direction field.]
enum OperationName {
option (gogoproto.goproto_enum_prefix) = false;

Expand All @@ -90,8 +89,13 @@ message HttpConnectionManager {
EGRESS = 1;
}

// The span name will be derived from this field.
OperationName operation_name = 1 [(validate.rules).enum.defined_only = true];
// The span name will be derived from this field. If
// :ref:`traffic_direction <envoy_api_field_Listener.traffic_direction>` is
// specified on the parent listener, then it is used instead of this field.
Copy link
Member

Choose a reason for hiding this comment

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

Should this be marked as deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should deprecate this rather than having this override thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't reliably tell if the field is present or not (due to a bad choice of the default).
That means we have to respect it until we can remove it from the API (which will happen according to the regular deprecation policy).

Copy link
Member

Choose a reason for hiding this comment

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

But you don't want to add [deprecated = true] to the field's options yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm OK adding deprecated = true, just waiting for the input from API shepherds.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, let's deprecate, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

//
// .. attention::
// This field has been deprecated in favor of `traffic_direction`.
OperationName operation_name = 1 [(validate.rules).enum.defined_only = true, deprecated = true];

// A list of header names used to create tags for the active span. The header name is used to
// populate the tag name, and the header value is used to populate the tag value. The tag is
Expand Down
5 changes: 5 additions & 0 deletions docs/root/intro/deprecated.rst
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ Version 1.12.0 (pending)
* The use of HTTP_JSON_V1 :ref:`Zipkin collector endpoint version
<envoy_api_field_config.trace.v2.ZipkinConfig.collector_endpoint_version>` or not explicitly
specifying it is deprecated, use HTTP_JSON or HTTP_PROTO instead.
* The `operation_name` field in :ref:`HTTP connection manager
<envoy_api_msg_config.filter.network.http_connection_manager.v2.HttpConnectionManager>`
has been deprecated in favor of the `traffic_direction` field in
:ref:`Listener <envoy_api_msg_Listener>`. The latter takes priority if
specified.

Version 1.11.0 (July 11, 2019)
==============================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,13 +244,27 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
Tracing::OperationName tracing_operation_name;
std::vector<Http::LowerCaseString> request_headers_for_tags;

switch (tracing_config.operation_name()) {
case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
Tracing::INGRESS:
// Listener level traffic direction overrides the operation name
switch (context.direction()) {
case envoy::api::v2::core::TrafficDirection::UNSPECIFIED: {
switch (tracing_config.operation_name()) {
case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
Tracing::INGRESS:
tracing_operation_name = Tracing::OperationName::Ingress;
break;
case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
Tracing::EGRESS:
tracing_operation_name = Tracing::OperationName::Egress;
break;
default:
NOT_REACHED_GCOVR_EXCL_LINE;
}
break;
}
case envoy::api::v2::core::TrafficDirection::INBOUND:
tracing_operation_name = Tracing::OperationName::Ingress;
break;
case envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager::
Tracing::EGRESS:
case envoy::api::v2::core::TrafficDirection::OUTBOUND:
tracing_operation_name = Tracing::OperationName::Egress;
break;
default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,62 @@ stat_prefix: router
EXPECT_EQ(5 * 60 * 1000, config.streamIdleTimeout().count());
}

TEST_F(HttpConnectionManagerConfigTest, ListenerDirectionOutboundOverride) {
const std::string yaml_string = R"EOF(
stat_prefix: router
route_config:
virtual_hosts:
- name: service
domains:
- "*"
routes:
- match:
prefix: "/"
route:
cluster: cluster
tracing:
operation_name: ingress
http_filters:
- name: envoy.router
config: {}
)EOF";

ON_CALL(context_, direction())
.WillByDefault(Return(envoy::api::v2::core::TrafficDirection::OUTBOUND));
HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
date_provider_, route_config_provider_manager_,
scoped_routes_config_provider_manager_);
EXPECT_EQ(Tracing::OperationName::Egress, config.tracingConfig()->operation_name_);
}

TEST_F(HttpConnectionManagerConfigTest, ListenerDirectionInboundOverride) {
const std::string yaml_string = R"EOF(
stat_prefix: router
route_config:
virtual_hosts:
- name: service
domains:
- "*"
routes:
- match:
prefix: "/"
route:
cluster: cluster
tracing:
operation_name: egress
http_filters:
- name: envoy.router
config: {}
)EOF";

ON_CALL(context_, direction())
.WillByDefault(Return(envoy::api::v2::core::TrafficDirection::INBOUND));
HttpConnectionManagerConfig config(parseHttpConnectionManagerFromV2Yaml(yaml_string), context_,
date_provider_, route_config_provider_manager_,
scoped_routes_config_provider_manager_);
EXPECT_EQ(Tracing::OperationName::Ingress, config.tracingConfig()->operation_name_);
}

TEST_F(HttpConnectionManagerConfigTest, SamplingDefault) {
const std::string yaml_string = R"EOF(
stat_prefix: ingress_http
Expand Down