Skip to content
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

Merged
merged 4 commits into from
Aug 5, 2021

Conversation

yashykt
Copy link
Member

@yashykt yashykt commented Aug 5, 2021

Updates based on https://github.com/grpc/proposal/pull/254/files

  1. Per-attempt span name - "Attempt.<service_name>.<method_name>"
  2. Add two attributes to the per-attempt span -
    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.

@yashykt yashykt force-pushed the OpenCensusCallAttemptSpanName branch from a1b71bb to 3642ee6 Compare August 5, 2021 17:48
@yashykt yashykt added the release notes: no Indicates if PR should not be in release notes label Aug 5, 2021
@yashykt yashykt changed the title Open census call attempt span name Open census call attempt span name and attribute changes Aug 5, 2021
@yashykt yashykt added release notes: yes Indicates if PR needs to be in release notes lang/c++ and removed release notes: no Indicates if PR should not be in release notes labels Aug 5, 2021
@yashykt yashykt requested a review from markdroth August 5, 2021 18:00
Copy link
Member

@markdroth markdroth left a 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.

src/cpp/ext/filters/census/client_filter.cc Outdated Show resolved Hide resolved
&parent_->context_.Span(),
parent_->context_.tags())),
start_time_(absl::Now()) {
GPR_DEBUG_ASSERT(parent_->context_.Context().IsValid());
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to StartNewAttempt

Copy link
Member

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_)),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works!

Copy link
Member

@markdroth markdroth left a 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.

@yashykt
Copy link
Member Author

yashykt commented Aug 5, 2021

Known issues: #26595, #26885
The interop cloud-to-cloud test timeout seems to be unrelated.

@yashykt yashykt merged commit 1b3e154 into grpc:master Aug 5, 2021
yashykt added a commit to yashykt/grpc that referenced this pull request Aug 5, 2021
* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
yashykt added a commit that referenced this pull request Aug 9, 2021
…6902)

* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
dennycd pushed a commit to dennycd/grpc that referenced this pull request Aug 10, 2021
…grpc#26902)

* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
yashykt added a commit that referenced this pull request Aug 10, 2021
…6957)

* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
dennycd pushed a commit to dennycd/grpc that referenced this pull request Aug 11, 2021
…grpc#26902)

* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
dennycd pushed a commit to dennycd/grpc that referenced this pull request Aug 11, 2021
…grpc#26902)

* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
Vignesh2208 pushed a commit to Vignesh2208/grpc that referenced this pull request Aug 20, 2021
* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
Vignesh2208 pushed a commit to Vignesh2208/grpc that referenced this pull request Aug 20, 2021
…grpc#26902)

* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request Sep 23, 2021
* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request Sep 23, 2021
…grpc#26902)

* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
lidizheng pushed a commit to lidizheng/grpc that referenced this pull request Sep 23, 2021
…grpc#26902)

* C++ OpenCensus filter: Add attributes to per-attempt span

* Reviewer comments

* Reviewer comment

* Remove extraneous assert
@yashykt yashykt deleted the OpenCensusCallAttemptSpanName branch May 18, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/c++ release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants