Add Span.Kind to the trace API. #1223

bogdandrutu · 2018-05-31T17:26:39Z

Partially fix #1054

codecov-io · 2018-05-31T17:41:58Z

Codecov Report

Merging #1223 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1223      +/-   ##
============================================
+ Coverage     81.96%   82.01%   +0.04%     
- Complexity     1233     1236       +3     
============================================
  Files           193      193              
  Lines          6017     6026       +9     
  Branches        558      558              
============================================
+ Hits           4932     4942      +10     
+ Misses          937      936       -1     
  Partials        148      148

Impacted Files	Coverage Δ	Complexity Δ
api/src/main/java/io/opencensus/trace/Span.java	`82.5% <100%> (+1.41%)`	`12 <0> (ø)`	⬇️
...main/java/io/opencensus/trace/export/SpanData.java	`95.23% <100%> (+0.11%)`	`7 <1> (+1)`	⬆️
...src/main/java/io/opencensus/trace/SpanBuilder.java	`100% <100%> (+6.25%)`	`4 <0> (ø)`	⬇️
...in/java/io/opencensus/implcore/trace/SpanImpl.java	`92.61% <100%> (+0.07%)`	`55 <1> (+1)`	⬆️
.../io/opencensus/implcore/trace/SpanBuilderImpl.java	`96.15% <100%> (+0.1%)`	`27 <1> (+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 8adb02c...87bf1d5. Read the comment docs.

songy23

Please add this change to CHANGELOG.md.

songy23 · 2018-05-31T17:59:56Z

api/src/main/java/io/opencensus/trace/export/SpanData.java

+  /**
+   * Returns a new immutable {@code SpanData}.
+   *
+   * @deprecated Use the version that contains the {@code Kind}.


suggestion: you can add a Javadoc link to the new method instead.

songy23 · 2018-05-31T18:02:11Z

api/src/main/java/io/opencensus/trace/export/SpanData.java

@@ -111,6 +149,7 @@ public static SpanData create(
        parentSpanId,
        hasRemoteParent,
        name,
+        kind,


Should we checkNotNull() here (since the arg Kind kind of this method is not marked Nullable)?

Made it @nullable. Actually null is a valid value.

Should SpanBuilder.setSpanKind also use @Nullable?

songy23 · 2018-05-31T18:02:38Z

impl_core/src/main/java/io/opencensus/implcore/trace/SpanBuilderImpl.java

+
+  @Override
+  public SpanBuilderImpl setSpanKind(Kind kind) {
+    this.kind = kind;


Same, checkNotNull() here?

songy23 · 2018-05-31T18:04:34Z

impl_core/src/main/java/io/opencensus/implcore/trace/SpanImpl.java

+   *
+   * @return the kind of this {@code Span}.
+   */
+  public Kind getKind() {


@Nullable?

codefromthecrypt · 2018-06-01T00:01:57Z

api/src/main/java/io/opencensus/trace/Span.java

+     * Indicates that the span covers server-side handling of an RPC or other remote network
+     * request.
+     */
+    SERVER,


/me wants PRODUCER, CONSUMER for christmas, santa

We should continue our discussion here census-instrumentation/opencensus-specs#54

bogdandrutu · 2018-06-01T01:08:59Z

PTAL.

songy23

LGTM

sebright · 2018-06-01T01:14:54Z

api/src/main/java/io/opencensus/trace/Span.java

+     * Indicates that the span covers server-side handling of an RPC or other remote network
+     * request.
+     */
+    SERVER,


The enum constants should also have @since tags, for when we add more.

sebright · 2018-06-01T01:38:08Z

api/src/main/java/io/opencensus/trace/export/SpanData.java

@@ -111,6 +149,7 @@ public static SpanData create(
        parentSpanId,
        hasRemoteParent,
        name,
+        kind,


Should SpanBuilder.setSpanKind also use @Nullable?

sebright · 2018-06-01T01:42:52Z

api/src/main/java/io/opencensus/trace/SpanBuilder.java

   * @return this.
   * @since 0.5
   */
  public abstract SpanBuilder setRecordEvents(boolean recordEvents);

+  /**
+   * Sets the {@link Span.Kind} for the newly created {@code Span}. If not called, the
+   * implementation will provide a default.


Can the default be null? It looks like this implementation doesn't set a non-null default.

Yes, null is a valid option. we can do unspecified if really is important but I think null is fine.

Shouldn't we specify that the default is null then? Allowing the implementation to use CLIENT or SERVER as a default doesn't seem correct.

sebright · 2018-06-01T01:46:50Z

api/src/main/java/io/opencensus/trace/SpanBuilder.java

+   * @return this.
+   * @since 0.14
+   */
+  public abstract SpanBuilder setSpanKind(Span.Kind spanKind);


Should this method be non-abstract, to avoid breaking existing subclasses?

codefromthecrypt · 2018-06-03T00:22:27Z

agree null as default

bogdandrutu requested review from songy23 and sebright May 31, 2018 17:26

googlebot added the cla: yes label May 31, 2018

songy23 reviewed May 31, 2018

View reviewed changes

songy23 assigned bogdandrutu May 31, 2018

bogdandrutu force-pushed the spankind branch from 45cfc86 to 8b1acc7 Compare May 31, 2018 18:07

sebright self-assigned this May 31, 2018

codefromthecrypt reviewed Jun 1, 2018

View reviewed changes

Bogdan Drutu added 3 commits May 31, 2018 18:07

Add Span.Kind to the trace API.

62177ca

Add @nullable annotation where needed.

657741c

Add changes to changelog.

87bf1d5

bogdandrutu force-pushed the spankind branch from 8b1acc7 to 87bf1d5 Compare June 1, 2018 01:08

songy23 approved these changes Jun 1, 2018

View reviewed changes

bogdandrutu merged commit 55c1277 into census-instrumentation:master Jun 1, 2018

bogdandrutu deleted the spankind branch June 1, 2018 01:56

sebright reviewed Jun 1, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Span.Kind to the trace API. #1223

Add Span.Kind to the trace API. #1223

bogdandrutu commented May 31, 2018

codecov-io commented May 31, 2018 •

edited

Loading

songy23 left a comment

songy23 May 31, 2018

bogdandrutu Jun 1, 2018

songy23 May 31, 2018

bogdandrutu Jun 1, 2018

sebright Jun 1, 2018

songy23 May 31, 2018

songy23 May 31, 2018

bogdandrutu Jun 1, 2018

codefromthecrypt Jun 1, 2018

bogdandrutu Jun 1, 2018

bogdandrutu commented Jun 1, 2018

songy23 left a comment

sebright Jun 1, 2018

sebright Jun 1, 2018

sebright Jun 1, 2018

bogdandrutu Jun 1, 2018

sebright Jun 1, 2018

sebright Jun 1, 2018

codefromthecrypt commented Jun 3, 2018 via email

Add Span.Kind to the trace API. #1223

Add Span.Kind to the trace API. #1223

Conversation

bogdandrutu commented May 31, 2018

codecov-io commented May 31, 2018 • edited Loading

Codecov Report

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Jun 1, 2018

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Jun 3, 2018 via email

codecov-io commented May 31, 2018 •

edited

Loading