-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #2989 to further look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also rename directory structure from http_tracers -> tracers as these
traces can and will be eventually used for things other than HTTP.