repo reorg: move tracers #2983
repo reorg: move tracers #2983
Conversation
Also rename directory structure from http_tracers -> tracers as these traces can and will be eventually used for things other than HTTP. Signed-off-by: Matt Klein <mklein@lyft.com>
| @@ -106,12 +106,7 @@ void MainImpl::initializeTracers(const envoy::config::trace::v2::Tracing& config | |||
|
|
|||
| // Now see if there is a factory that will accept the config. | |||
| auto& factory = Config::Utility::getAndCheckFactory<HttpTracerFactory>(type); | |||
| if (factory.requiresClusterName() && server.localInfo().clusterName().empty()) { | |||
There was a problem hiding this comment.
reviewer note: This is the only real code change in here. I removed this check to avoid having to end up implementing a fake trace driver just to test this. When I looked into this, I don't think this check has any real purpose. This information is used only to set tags, and we don't enforce other aspects of local info which are also used to set tags. I think it's OK for them to be empty and then have empty tags. (It also seemed pretty weird that the dynamic trace drivers would not care about this for example.)
There was a problem hiding this comment.
I opened #2989 to further look into this.
There was a problem hiding this comment.
If you dynamically load a tracer, the connection to the collector is made within the tracing library and specified in the tracer's configuration, instead of being managed by envoy. So there wouldn't be any clusterName in that case.
The check BTW was already there. I only modified it so that it's disabled for the case of dynamically loaded tracers.
There was a problem hiding this comment.
Yeah, the check is actually not for target cluster, it's actually for the local cluster, which ends up being used in tags (zipkin and LS have other checks for the target cluster existing). I should have caught this during your review but I didn't. I think it's fine to remove this check and we an revisit in the linked ticket if we want to have guards on various "local info" being available.
| "--service-cluster option.", | ||
| type)); | ||
| } | ||
| http_tracer_ = factory.createHttpTracer(*driver_config, server, *cluster_manager_); |
There was a problem hiding this comment.
This is a code change, but it removes a redundant param which is not needed. Cluster manager is contained in the server.
Signed-off-by: Matt Klein <mklein@lyft.com>
| @@ -106,12 +106,7 @@ void MainImpl::initializeTracers(const envoy::config::trace::v2::Tracing& config | |||
|
|
|||
| // Now see if there is a factory that will accept the config. | |||
| auto& factory = Config::Utility::getAndCheckFactory<HttpTracerFactory>(type); | |||
| if (factory.requiresClusterName() && server.localInfo().clusterName().empty()) { | |||
Also rename directory structure from http_tracers -> tracers as these
traces can and will be eventually used for things other than HTTP.