WIP: Opencensus integration

[igorbernstein2]

DON'T MERGE

This is a rough sketch of opencensus integration. I wanted to get some early feedback on the direction I'm taking.

The general idea here is to create an opencensus span for each toplevel operation the user invokes. Then to annotate that span with various interesting events like retry attempts.

All of the logic to create spans and annotations is centralized in a Tracer. A Tracer wraps an opencensus Span. The Tracer is generally created by outer callables like TracedUnaryCallable and is passed through the callable chains via the ApiCallContext.

The main things I would like to get some feedback on are:

1. Tracer interface: it centralizes all of the annotations as opposed to exposing the raw opencensus apis.
2. The statefulness of the OpencensusTracer impl. It's has some smarts like counting requests and responses that it will use in a future annotation.
3. Keeping all tracing related classes in the tracing package (including the Callable impls)
4. The addition of couldRetry to the RetryAlgorithm` to disambiguate between running out of retry attempts and the error not being retryable

TODO

• Finish implementation
• Add tracing for HTTP
• Add credentials refresh annotations
• Figure out what to do about traces generated by opentracing stubs

[codecov-io]

Codecov Report

Merging #613 into master will decrease coverage by 5.02%.
The diff coverage is 19.75%.

@@             Coverage Diff              @@
##             master     #613      +/-   ##
============================================
- Coverage     75.03%   70.01%   -5.03%     
- Complexity      935      957      +22     
============================================
  Files           177      190      +13     
  Lines          4090     4492     +402     
  Branches        323      334      +11     
============================================
+ Hits           3069     3145      +76     
- Misses          869     1192     +323     
- Partials        152      155       +3

Impacted Files	Coverage Δ	Complexity Δ
...java/com/google/api/gax/tracing/TraceFinisher.java	0% <0%> (ø)	0 <0> (?)
...api/gax/tracing/TracedClientStreamingCallable.java	0% <0%> (ø)	0 <0> (?)
...oogle/api/gax/tracing/OpencensusTracerFactory.java	0% <0%> (ø)	0 <0> (?)
...api/gax/tracing/TracedServerStreamingCallable.java	0% <0%> (ø)	0 <0> (?)
...a/com/google/api/gax/tracing/OpencensusTracer.java	0% <0%> (ø)	0 <0> (?)
...main/java/com/google/api/gax/tracing/SpanName.java	0% <0%> (ø)	0 <0> (?)
...oogle/api/gax/tracing/TracedOperationCallable.java	0% <0%> (ø)	0 <0> (?)
...om/google/api/gax/tracing/TracedUnaryCallable.java	0% <0%> (ø)	0 <0> (?)
...om/google/api/gax/tracing/TracedBatchCallable.java	0% <0%> (ø)	0 <0> (?)
...com/google/api/gax/tracing/TracedBidiCallable.java	0% <0%> (ø)	0 <0> (?)
... and 27 more

---

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 0eb9430...df1c6b5. Read the comment docs.

[vam-google]

I did not review the actual implementations for TracedCallables (as it is probably too early to do it). Please let me know if you want those to be carefully reviewed as well.

About the 4 specific questions:

1. Tracer interface is fine, maybe except Type enum in it (please see the corresponding comment for details).
2. Please avoid the duplication of TimedAttemptSettings stuff at all cost (please check the corresponding comment in OpencensusTracer class). I'm not strictly opposed to the statefulness of the tracer, but I'm opposed to duplicating in it the state of other objects, because there would be no guarantee that the reported values are the actual values.
3. gax.tracing. package is fine (we are already structured as package-by-feature). As for Traced*Callables, please check the comment in GrpcCallableFactory (I think it is ok to just not have them at all, and merge all the tracing stuff into existing callables).
4. couldRetry - I really don't like it (especially when there is shouldRetry next to it). It looks like you can avoid declaring this method (and in a way, which will give you more flexibility in terms of reporting). Please check the corresponding comments for details.

[igorbernstein2]

Thanks for reviewing, I will incorporate most of the suggestions over the coarse of the week. However there are couple things that I think I need to keep:

1. I need to retain the Traced callables to actually start the operation spans. There is currently no existing callable that I can modify to insert that functionality. Also I think it's good practice to separate concerns (ie. having a ExceptionCallable separate from direct callable)
2. I would like to keep the wrapping instrumentation approach. The only place where I couldn't use that approach was the retry logic. I would prefer to update the retrying control flow to allow it to be wrapped instead. Maybe pushing the decision to RetryAlgorithm and have it return descriptive state that can be intercepted.
3. I would like to be keep the ApiTracer abstraction instead of exposing the opencensus Tracer directly. The interface of the ApiTracer will be a good reference to understand the kind of things being traced
4. I think I need to keep the enum to distinguish between retry attempts vs polling attempts in my annotations.