-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Open census call attempt span name and attribute changes #26889
Conversation
a1b71bb
to
3642ee6
Compare
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.
Looks great -- just a couple of minor nits.
&parent_->context_.Span(), | ||
parent_->context_.tags())), | ||
start_time_(absl::Now()) { | ||
GPR_DEBUG_ASSERT(parent_->context_.Context().IsValid()); |
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.
Prior to this PR, this assert would be run before passing the fields from parent_->context_
to the CensusContext
ctor, but now it's being done after. Is that okay?
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.
moved to StartNewAttempt
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.
That's not quite optimal, since someone could later change the code to instantiate OpenCensusCallAttemptTracer
from somewhere else, and they might not remember to add the assertion.
Is CensusContext
movable? If so, you can use a helper function like this:
namespace {
CensusContext CreateCensusContextForCallAttempt(
absl::string_view method, const CensusContext& parent_context) {
GPR_DEBUG_ASSERT(parent_context.Context().IsValid());
return CensusContext(method, &parent_context.Span(), parent_context.tags());
}
} // namespace
Then in the initializer list, you can do this:
context_(CreateCensusContextForCallAttempt(
absl::StrCat("Attempt.", parent_->method_),
parent_->context_)),
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.
works!
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.
Thanks for doing this!
Please back-port to 1.40.
* C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
…grpc#26902) * C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
…grpc#26902) * C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
…grpc#26902) * C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
* C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
…grpc#26902) * C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
* C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
…grpc#26902) * C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
…grpc#26902) * C++ OpenCensus filter: Add attributes to per-attempt span * Reviewer comments * Reviewer comment * Remove extraneous assert
Updates based on https://github.com/grpc/proposal/pull/254/files
i)
previous-rpc-attempts
- an integer value, number of preceding attempts, transparent retry not included.ii)
is-transparent-retry
- a boolean value, whether the attempt is a transparent retry.