Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Opencensus Tracing: Add annotations for retries #640

Merged
igorbernstein2 merged 8 commits into from
Jan 17, 2019
Merged

Opencensus Tracing: Add annotations for retries #640

igorbernstein2 merged 8 commits into from
Jan 17, 2019

Conversation

@igorbernstein2
Copy link
Contributor

@igorbernstein2 igorbernstein2 commented Jan 8, 2019 •
edited
Loading

This instruments the retry logic to notify the tracer of the retry lifecycle. It also fixed a bug in the ApiTracer interface that prevented exhausted retries from passing the error.

@igorbernstein2
WIP: Add annotations for retries
2e8d024 
This instruments the retry logic to notify the tracer of the retry lifecycle. It also fixed a bug in the ApiTracer interface that prevented exhausted retries from passing the error.
@igorbernstein2
fix tests
0d2ee97
@igorbernstein2 igorbernstein2 added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 8, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 8, 2019
@codecov
Copy link

codecov bot commented Jan 8, 2019 •
edited
Loading

Codecov Report

Merging #640 into master will increase coverage by 0.25%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #640      +/-   ##
============================================
+ Coverage     74.56%   74.82%   +0.25%     
- Complexity      949      959      +10     
============================================
  Files           180      181       +1     
  Lines          4172     4198      +26     
  Branches        334      335       +1     
============================================
+ Hits           3111     3141      +30     
+ Misses          908      904       -4     
  Partials        153      153
Impacted Files Coverage Δ Complexity Δ
...le/api/gax/rpc/ServerStreamingAttemptCallable.java 87.15% <100%> (+0.36%) 17 <0> (ø) ⬇️
...m/google/api/gax/retrying/BasicRetryingFuture.java 93.61% <100%> (+0.75%) 23 <7> (+1) ⬆️
...java/com/google/api/gax/tracing/NoopApiTracer.java 55.55% <100%> (+55.55%) 8 <1> (+8) ⬆️
...n/java/com/google/api/gax/rpc/AttemptCallable.java 82.6% <100%> (+2.6%) 4 <0> (ø) ⬇️
...om/google/api/gax/rpc/CheckingAttemptCallable.java 81.81% <100%> (+2.87%) 4 <0> (ø) ⬇️
.../com/google/api/gax/grpc/GrpcTransportChannel.java 59.09% <0%> (-13.14%) 9% <0%> (ø)
...om/google/api/gax/core/ResourceCloseException.java 0% <0%> (ø) 0% <0%> (?)
...va/com/google/api/gax/retrying/RetryAlgorithm.java 81.25% <0%> (+6.25%) 8% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ce5a2f...7f0bcab. Read the comment docs.

@igorbernstein2 igorbernstein2 changed the title WIP: Add annotations for retries Add annotations for retries Jan 8, 2019
@igorbernstein2 igorbernstein2 removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jan 8, 2019
vam-google
vam-google reviewed Jan 10, 2019
View reviewed changes
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

Mostly looks good, with one real concern about the notion of cancellation in ApiTracer (see corresponding comment in BasicRetryingFuture.

gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java Outdated
clearAttemptServiceData();
if (throwable instanceof CancellationException) {
// An attempt triggered cancellation.
retryingContext.getTracer().attemptFailedRetriesExhausted(throwable);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java Outdated
clearAttemptServiceData();
if (throwable instanceof CancellationException) {
// An attempt triggered cancellation.
retryingContext.getTracer().attemptFailedRetriesExhausted(throwable);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

gax/src/test/java/com/google/api/gax/retrying/AbstractRetryingExecutorTest.java Outdated
assertFutureSuccess(future);
assertEquals(0, future.getAttemptSettings().getAttemptCount());

Mockito.verify(tracer, Mockito.times(1)).attemptStarted(0);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

gax/src/test/java/com/google/api/gax/retrying/FailingCallable.java Outdated
private final String result;

FailingCallable(int expectedFailuresCount, String result) {
FailingCallable(ApiTracer tracer, int expectedFailuresCount, String result) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@igorbernstein2
Copy link
Contributor Author

igorbernstein2 commented Jan 10, 2019

All feedback should be addressed, PTAL

@igorbernstein2 igorbernstein2 mentioned this pull request Jan 10, 2019
Merged
@igorbernstein2 igorbernstein2 changed the title Add annotations for retries Opencensus Tracing: Add annotations for retries Jan 11, 2019
vam-google
vam-google approved these changes Jan 17, 2019
View reviewed changes
Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

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

LGTM (with minor comment)

gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java
// the current attempt.
// In a theoretical scenario, the attempt callable might've thrown the exception on its
// own volition. However it's currently impossible to disambiguate the 2 scenarios.
tracer.attemptCancelled();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@igorbernstein2
Copy link
Contributor Author

igorbernstein2 commented Jan 17, 2019

Thanks for the review!

@igorbernstein2 igorbernstein2 merged commit 1dd8124 into googleapis:master Jan 17, 2019
@igorbernstein2 igorbernstein2 deleted the oc-retries branch January 17, 2019 19:20
This was referenced Jan 31, 2019
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@vam-google vam-google vam-google approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@igorbernstein2 @vam-google @googlebot