tracing: plumb HttpTracer into AsyncClientImpl#888
Conversation
mattklein123
left a comment
There was a problem hiding this comment.
at a high level looks good, some small comments.
| #include "envoy/upstream/cluster_manager.h" | ||
|
|
||
| #include "common/common/logger.h" | ||
| #include "common/tracing/http_tracer_impl.h" |
There was a problem hiding this comment.
shouldn't need impl header here, just interface.
| public: | ||
| virtual ~HttpTracer() {} | ||
|
|
||
| virtual void installDriver(DriverPtr&& driver) PURE; |
There was a problem hiding this comment.
I see why you need this in the config code, but I would prefer that we not expose this function around the code base. If you need it to be part of the interface, can we make another interface the derives from this interface that allows driver installation?
| HttpTracerImpl::HttpTracerImpl(DriverPtr&& driver, const LocalInfo::LocalInfo& local_info) | ||
| : driver_(std::move(driver)), local_info_(local_info) {} | ||
|
|
||
| void HttpTracerImpl::installDriver(DriverPtr&& driver) { driver_ = std::move(driver); } |
There was a problem hiding this comment.
No one should start a span between the time we make a tracer, and install the driver. I would remove driver from the constructor, and allow it to be null. Then in this function I would ASSERT(driver_ == nullptr);
|
Implemented above changes, but on hold pending related PR that supports an alternate approach. |
|
@goaway should we just close this? |
|
Sure, can always re-open if for some reason we decide we want it. |
* Remove 5 seconds sleep for integration tests. * Update per comment
**Description** This commit add OwnerReference for HTTPRouteFilters on creation. **Related Issues/PRs (if applicable)** Fixes: envoyproxy/ai-gateway#873 --------- Signed-off-by: googs1025 <googs1025@gmail.com>
No description provided.