tracing: allow tracers to be configured with a collector cluster added via CDS#10526
Conversation
…r added via CDS Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
…er added via CDS Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
…ster added via CDS Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
|
/retest |
|
🔨 rebuilding |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM at a high level with some small comments. Thanks for working on this!
/wait
| driver_.clusterManager() | ||
| .httpAsyncClientForCluster(driver_.cluster()->name()) | ||
| .send(std::move(message), *this, | ||
| if (driver_.clusterManager().get(driver_.cluster())) { |
There was a problem hiding this comment.
Optimally you would use the thread local cluster manager callbacks here to avoid this lookup every time. Perhaps put in a TODO for that if you don't want to do it now? It's possible that a helper base/mix-in class could be used for this across tracers and other similar cases. (Comment applies to all the other tracers.)
There was a problem hiding this comment.
I've implemented it for Zipkin tracer. Please take a look.
There was a problem hiding this comment.
I've implemented cluster manager callbacks for the remaining tracers.
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
…acks Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
…cers-to-be-configured-with-cds-clusters Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM other than a comment to share the callback code. Thank you!
/wait
| } | ||
| } | ||
|
|
||
| void TraceReporter::onClusterAddOrUpdate(Upstream::ThreadLocalCluster& cluster) { |
There was a problem hiding this comment.
Can you move this into some kind of base class/mix-in shared by each tracer? I think it should be pretty easy to do.
There was a problem hiding this comment.
done.
I had to sacrifice ENVOY_LOG(debug, ...) statement, though.
…mponent Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
#10526 allowed tracers to use CDS clusters. But zipkin proto doc still says bootstrap cluster is mandatory Risk Level: Low Testing: N/A Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
envoyproxy#10526 allowed tracers to use CDS clusters. But zipkin proto doc still says bootstrap cluster is mandatory Risk Level: Low Testing: N/A Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Signed-off-by: Qin Qin <qqin@google.com>
2 participants
Description: allow tracers to be configured with a collector cluster added via CDS
Risk Level: Medium
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A
Context: