tracing: proper child span support in gRPC client#1885
Conversation
- Remove finalizer concept as it isn't needed and requires extra allocations. - Plumb tracing through into gRPC client and wire up in ratelimit client. - Do not require null allocation for span in connection manager if tracing is not configured. - Remove propagating x-request-id for ratelimit requests as we now have proper tracing support. If this is requested in the future we should make request ID a part of Tracing::Span itself. Signed-off-by: Matt Klein <mklein@lyft.com>
|
@objectiser this will conflict with your PR I think. We can merge yours first. |
rshriram
left a comment
There was a problem hiding this comment.
Nice. Is it possible to apply this to the xDS calls as well ?
objectiser
left a comment
There was a problem hiding this comment.
Just some comments about the tags.
|
|
||
| struct { | ||
| const std::string STATUS = "status"; | ||
| const std::string GRPC_STATUS = "grpc-status"; |
There was a problem hiding this comment.
Could this be changed to grpc.status_code to be consistent with the http equivalent?
| void cancel() override { this->resetStream(); } | ||
| void cancel() override { | ||
| current_span_->setTag(TracingConfig::get().TagStrings.STATUS, | ||
| TracingConfig::get().TagStrings.CANCELED); |
There was a problem hiding this comment.
Should this record the status in grpc.status_code (TagStrings.GRPC_STATUS) as discussed above, and use the GRPC status code value i.e. CANCELLED?
There was a problem hiding this comment.
I thought about this, but my thinking is that this is a local cancel, not a remote cancel, so we shouldn't confuse it with gRPC remote cancellation?
There was a problem hiding this comment.
Good point - best to keep separate.
| callbacks_.onFailure(Status::Internal, EMPTY_STRING); | ||
| return; | ||
| current_span_->setTag(TracingConfig::get().TagStrings.ERROR, | ||
| TracingConfig::get().TagStrings.TRUE); |
There was a problem hiding this comment.
Should also record the status, TagStrings.GRPC_STATUS, status) (status in text form).
There was a problem hiding this comment.
We do store the numeric status above. Are you suggesting a new tag name with spelled out status?
There was a problem hiding this comment.
No, ignore - storing the integer is fine.
|
@objectiser updated |
Signed-off-by: Matt Klein <mklein@lyft.com>
|
LGTM. Basically what I was going for sans the finalizer overhead, which I agree ends up being an improvement. |
|
|
||
| current_span_ = parent_span.spawnChild(TracingConfig::get(), | ||
| "async " + parent.remote_cluster_name_ + " egress", | ||
| ProdSystemTimeSource::instance_.currentTime()); |
There was a problem hiding this comment.
reminds me that we need to upgrade lightstep to add monotonic time to tracing
There was a problem hiding this comment.
Yeah we do need to do this. I think that entire driving is being worked on so hopefully we can get it swapped out with the OT driver update.
| class AsyncClientTracingConfig : public Tracing::Config { | ||
| public: | ||
| // Tracing::Config | ||
| Tracing::OperationName operationName() const override { return Tracing::OperationName::Egress; } |
There was a problem hiding this comment.
missing cov for :265-:268
|
@danielhochman @goaway updated |
* Reduce log level for jwt filter (envoyproxy#1866) * Update_Dependencies (envoyproxy#1873) * Correctly clean up headers used for payload from JWT authentication (envoyproxy#1879) * Correctly clean up headers used for payload from JWT authentication * Clang * Update_Dependencies (envoyproxy#1883) * destination.principal derivation fix (envoyproxy#1884) * fix attribute extraction Signed-off-by: Kuat Yessenov <kuat@google.com> * seed mock Signed-off-by: Kuat Yessenov <kuat@google.com> * merge 1.0 to master * Update API SHA (envoyproxy#1891) * add needed dependencies for circle ci
Description: Assign an int to each log level for consumption. Risk Level: low Testing: ci Docs Changes: n/a Release Notes: n/a [Optional Fixes #Issue] n/a [Optional Deprecated:] n/a Signed-off-by: Jingwei Hao <jingwei.hao@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Description: Assign an int to each log level for consumption. Risk Level: low Testing: ci Docs Changes: n/a Release Notes: n/a [Optional Fixes #Issue] n/a [Optional Deprecated:] n/a Signed-off-by: Jingwei Hao <jingwei.hao@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
allocations.
client.
if tracing is not configured.
now have proper tracing support. If this is requested in the
future we should make request ID a part of Tracing::Span
itself.
Signed-off-by: Matt Klein mklein@lyft.com