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.

Sign up for GitHub

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

Init Http::ContextImpl::tracer_ when tracing is enabled, and test it. #5249

Merged
merged 2 commits into from
Dec 8, 2018
Merged

Init Http::ContextImpl::tracer_ when tracing is enabled, and test it. #5249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f383cb8
Init Http::ContextImpl::tracer_ when tracing is enabled, and test it.
jmarantz Dec 8, 2018
97a983e
Remove jaeger-tracer testcase.
jmarantz Dec 8, 2018
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
1 change: 1 addition & 0 deletions source/server/server.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ void InstanceImpl::initialize(Options& options,
// is constructed as part of the InstanceImpl and then populated once
// cluster_manager_factory_ is available.
config_.initialize(bootstrap_, *this, *cluster_manager_factory_);
http_context_.setTracer(config_.httpTracer());

// Instruct the listener manager to create the LDS provider if needed. This must be done later
// because various items do not yet exist when the listener manager is created.
Expand Down
2 changes: 2 additions & 0 deletions test/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,7 @@ envoy_cc_test(
":node_bootstrap.yaml",
":node_bootstrap_no_admin_port.yaml",
":node_bootstrap_without_access_log.yaml",
":zipkin_tracing.yaml",
"//test/config/integration:server.json",
"//test/config/integration:server_config_files",
],
Expand All @@ -218,6 +219,7 @@ envoy_cc_test(
"//source/extensions/filters/network/http_connection_manager:config",
"//source/extensions/filters/network/redis_proxy:config",
"//source/extensions/stat_sinks/statsd:config",
"//source/extensions/tracers/zipkin:config",
"//source/server:server_lib",
"//test/integration:integration_lib",
"//test/mocks/server:server_mocks",
Expand Down
14 changes: 14 additions & 0 deletions test/server/server_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,5 +378,19 @@ TEST_P(ServerInstanceImplTest, MutexContentionEnabled) {
EXPECT_NO_THROW(initialize(std::string()));
}

TEST_P(ServerInstanceImplTest, NoHttpTracing) {
options_.service_cluster_name_ = "some_cluster_name";
options_.service_node_name_ = "some_node_name";
EXPECT_NO_THROW(initialize("test/server/empty_bootstrap.yaml"));
EXPECT_NE(nullptr, dynamic_cast<Tracing::HttpNullTracer*>(&server_->httpContext().tracer()));
}

TEST_P(ServerInstanceImplTest, ZipkinHttpTracingEnabled) {
options_.service_cluster_name_ = "some_cluster_name";
options_.service_node_name_ = "some_node_name";
EXPECT_NO_THROW(initialize("test/server/zipkin_tracing.yaml"));
EXPECT_EQ(nullptr, dynamic_cast<Tracing::HttpNullTracer*>(&server_->httpContext().tracer()));
Copy link
Member

Choose a reason for hiding this comment

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

It might be a slightly better test to check for the Zipkin tracer type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed...I probably should have looked deeper. Is the zipkin tracer object exposed?

In any case I could follow up with an edit tomorrow if you want to get the fix in sooner.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's a concrete type just like the other tracers. If the config loads we must be compiling in support so it should be available in a header. Sure let's merge now since this is pretty broken as-is and can do a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem like there's a specific Zipkin class for this but I added a test for casting to HttpTracerImpl* in #5250.

}

} // namespace Server
} // namespace Envoy
10 changes: 10 additions & 0 deletions test/server/zipkin_tracing.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
static_resources:
clusters:
- name: zipkin
connect_timeout: 1s
tracing:
http:
name: envoy.zipkin
config:
collector_cluster: zipkin
collector_endpoint: "/api/v1/spans"