Stats/Exemplar: Add a generic AttachmentValue class. by songy23 · Pull Request #1779 · census-instrumentation/opencensus-java

songy23 · 2019-02-28T23:02:32Z

Updates #1775.

songy23 · 2019-02-28T23:03:10Z

This is still WIP, but before I move on to implementation I'd like to get some early feedback on how the API looks like

songy23 · 2019-03-01T01:10:26Z

...mplar_util/src/main/java/io/opencensus/contrib/exemplar/util/AttachmentValueSpanContext.java

+ */
+@Immutable
+@AutoValue
+public abstract class AttachmentValueSpanContext extends AttachmentValue {


The idea is we cannot have Trace APIs dependency in Stats APIs, but we can extend the generic abstract class in a contrib util package and have the util depend on both Trace and Stats APIs.

That is correct. Maybe put this in comment. Also say we try to avoid having the Trace API ....

bogdandrutu · 2019-03-01T02:13:06Z

...mplar_util/src/main/java/io/opencensus/contrib/exemplar/util/AttachmentValueSpanContext.java

+ */
+@Immutable
+@AutoValue
+public abstract class AttachmentValueSpanContext extends AttachmentValue {


That is correct. Maybe put this in comment. Also say we try to avoid having the Trace API ....

bogdandrutu · 2019-03-01T02:21:42Z

api/src/main/java/io/opencensus/stats/AttachmentValue.java

+   *
+   * @since 0.20
+   */
+  public abstract <T> T match(


I don't think the match pattern is a good option here. Just have a:

public abstract String getValue();

We can do a bit of a hack in the exporter and have:

if (value instanceof AttachmentValueSpanContext) { AttachmentValueSpanContext avsc = (AttachmentValueSpanContext) value; avsc.getSpanContext(); }

Agree that match pattern is not very useful in this case since there's only one subclass in the API. Having a getValue instead makes sense to me.

codecov-io · 2019-03-02T00:09:00Z

Codecov Report

Merging #1779 into master will decrease coverage by 0.08%.
The diff coverage is 64%.

@@             Coverage Diff              @@
##             master    #1779      +/-   ##
============================================
- Coverage     82.45%   82.36%   -0.09%     
- Complexity     1993     1995       +2     
============================================
  Files           291      293       +2     
  Lines          9221     9234      +13     
  Branches        891      891              
============================================
+ Hits           7603     7606       +3     
- Misses         1320     1329       +9     
- Partials        298      299       +1

Impacted Files	Coverage Δ	Complexity Δ
...a/io/opencensus/implcore/stats/IntervalBucket.java	`100% <ø> (ø)`	`11 <0> (ø)`	⬇️
...a/io/opencensus/implcore/stats/MeasureMapImpl.java	`100% <ø> (ø)`	`11 <0> (ø)`	⬇️
.../io/opencensus/implcore/stats/MutableViewData.java	`88.46% <ø> (ø)`	`4 <0> (ø)`	⬇️
.../opencensus/implcore/stats/MutableAggregation.java	`89.79% <0%> (-1.24%)`	`0 <0> (ø)`
.../src/main/java/io/opencensus/stats/MeasureMap.java	`40% <0%> (-10%)`	`2 <0> (ø)`
...java/io/opencensus/implcore/stats/MetricUtils.java	`83.72% <0%> (-11.02%)`	`8 <0> (ø)`
...trib/exemplar/util/AttachmentValueSpanContext.java	`100% <100%> (ø)`	`3 <3> (?)`
...main/java/io/opencensus/stats/AttachmentValue.java	`100% <100%> (ø)`	`1 <1> (?)`
...io/opencensus/implcore/stats/MeasureToViewMap.java	`86.07% <100%> (ø)`	`26 <0> (ø)`	⬇️
.../opencensus/implcore/stats/MeasureMapInternal.java	`90.32% <100%> (+0.32%)`	`4 <1> (ø)`	⬇️
... and 7 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 7ce769e...b255eb9. Read the comment docs.

bogdandrutu · 2019-03-02T21:52:50Z

api/src/main/java/io/opencensus/stats/AggregationData.java

   * @since 0.8
   */
-  @Immutable
+  @javax.annotation.concurrent.Immutable


why full qualifier here?

Without it, nullness checker framework would complain:

/usr/local/google/home/songya/Google/opencensus-java/api/src/main/java/io/opencensus/stats/AggregationData.java:261: error: cannot find symbol @Immutable ^ symbol: class Immutable location: class AggregationData 1 error

@sebright Do you have any suggestion here?

I checked out this branch, removed "javax.annotation.concurrent.", and ran ./gradlew assemble -PcheckerFramework, but I couldn't reproduce the error. Did you make another change that affects the null analysis in this file?

See https://travis-ci.org/census-instrumentation/opencensus-java/jobs/501696694#L541 after 50a4033:

/home/travis/build/census-instrumentation/opencensus-java/api/src/main/java/io/opencensus/stats/AggregationData.java:261: error: cannot find symbol @Immutable ^ symbol: class Immutable location: class AggregationData 1 error > Task :opencensus-api:compileJava **FAILED**

I got the exact same error on my desktop by running ./gradlew clean assemble -PcheckerFramework.

Thanks. I can't reproduce it locally, but I'll keep looking into the failure on Travis.

Thanks, I filed #1782 and reverted the change in d537c30 for now.

bogdandrutu · 2019-03-02T21:58:43Z

api/src/main/java/io/opencensus/stats/AggregationData.java

+       * @return an {@code Exemplar}.
+       * @since 0.20
+       */
+      public static Exemplar createNew(


Why do you have a different name here?

Because we already have an API with the same signature:

opencensus-java/api/src/main/java/io/opencensus/stats/AggregationData.java

Lines 494 to 495 in ffd2176

public static Exemplar create(

double value, Timestamp timestamp, Map<String, String> attachments) {

The only difference is the type argument in Map and Java doesn't allow us to overload a method with just that.

Another option is to just remove the old API. This will be a breaking change but I don't think anyone except the Stats exporters are depending on this.

Same argument for getAttachments, what do you think? I think removing this is the best option, we don't break any instrumented code with this.

Agree that removing the deprecated APIs are the easiest. I won't expect any client code depending on those APIs since no backend supported Exemplar before. Going to make the change.

api/src/main/java/io/opencensus/stats/AggregationData.java

songy23 · 2019-03-05T17:58:58Z

impl_core/src/main/java/io/opencensus/implcore/stats/MetricUtils.java

    return labelValues;
  }

+  static Map<String, String> toStringAttachments(Map<String, AttachmentValue> attachments) {


For the moment Exemplars in Metrics still have <string, stirng> attachments: https://github.com/census-instrumentation/opencensus-proto/blob/c9cc5de7251f8a320dd6cb4f92e5698ba86f284c/src/opencensus/proto/metrics/v1/metrics.proto#L263.

For now we need to convert AttachmentValue in Stats API to string in Metrics API.

We can modify that because we respect the data model, the fact that we delay the string formatting doesn't mean we don't respect the proto. This is one of the reason we don't directly use the protos.

That would be tricky since AttachmentValue is part of Stats APIs and Metrics APIs cannot have dependency on Stats APIs. If we really want to replace the <string, string> attachments in Metrics APIs, the only option I can see is to put Exemplar and AttachmentValue in common artifacts or a top-level API (which Go already does: https://github.com/census-instrumentation/opencensus-go/tree/master/exemplar).

Either way I would prefer to do it in a separate PR since this one is already large.

Sure, agree, let's discuss in a separate PR.

bogdandrutu · 2019-03-06T21:22:25Z

Any reason to not merge this?

songy23 · 2019-03-06T21:46:43Z

@rghetia and @dinooliva mentioned they were also reviewing this PR - any comments/concerns on this? (More changes will follow in a follow-up PR.)

songy23 · 2019-03-07T18:38:00Z

Going to merge this PR by EOD today if there're no other comments. More refactoring is in #1791.

songy23 added DO NOT MERGE stats API labels Feb 28, 2019

songy23 requested review from dinooliva and rghetia as code owners February 28, 2019 23:02

googlebot added the cla: yes label Feb 28, 2019

songy23 requested a review from bogdandrutu February 28, 2019 23:03

songy23 force-pushed the exemplar-attachment-value branch from 3ed832d to 1fb8b23 Compare February 28, 2019 23:10

songy23 commented Mar 1, 2019

View reviewed changes

bogdandrutu reviewed Mar 1, 2019

View reviewed changes

songy23 requested a review from a team as a code owner March 1, 2019 23:23

songy23 force-pushed the exemplar-attachment-value branch from a6f1bed to f2854fc Compare March 1, 2019 23:46

bogdandrutu reviewed Mar 2, 2019

View reviewed changes

songy23 changed the title ~~[WIP] Stats/Exemplar: Add a generic AttachmentValue class.~~ Stats/Exemplar: Add a generic AttachmentValue class. Mar 4, 2019

songy23 removed the DO NOT MERGE label Mar 4, 2019

songy23 mentioned this pull request Mar 4, 2019

Investigate the error from checker framework with Immutable annotation. #1782

Closed

songy23 added 9 commits March 4, 2019 14:40

Stats/Exemplar: Add a generic AttachmentValue class.

2bac5f6

Add AttachmentValueSpanContext in cotrib repo.

9fb2ccd

Use getValue instead of match. Add more comments.

7f11abd

Add tests.

48ea568

Update implementations.

ed22998

Fix checker framework. Update deprecated APIs.

dcaaed9

Add a test on null timestamp.

89a5275

Test removing qualifiers for Immutable.

6a566b7

Revert removing qualifiers for Immutable.

d76abe3

songy23 force-pushed the exemplar-attachment-value branch from d537c30 to d76abe3 Compare March 4, 2019 22:40

Add to CHANGELOG.

40cceb1

songy23 added the kokoro:force-run Label to force a Kokoro build immediately. label Mar 4, 2019

kokoro-team removed the kokoro:force-run Label to force a Kokoro build immediately. label Mar 4, 2019

Remove deprecated APIs.

b255eb9

songy23 commented Mar 5, 2019

View reviewed changes

bogdandrutu approved these changes Mar 5, 2019

View reviewed changes

songy23 mentioned this pull request Mar 7, 2019

Exemplar: Refactor to be under metrics.data. #1791

Merged

songy23 merged commit 451948a into census-instrumentation:master Mar 8, 2019

songy23 deleted the exemplar-attachment-value branch March 8, 2019 02:09

This was referenced Mar 13, 2019

Stats: Consider providing a convenient API for recording with SpanContext attachment. #1775

Closed

Exemplar: Use generic interface for attachment values. census-instrumentation/opencensus-go#1070

Merged

	public static Exemplar create(
	double value, Timestamp timestamp, Map<String, String> attachments) {

Conversation

songy23 commented Feb 28, 2019

Uh oh!

songy23 commented Feb 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdandrutu commented Mar 6, 2019

Uh oh!

songy23 commented Mar 6, 2019

Uh oh!

songy23 commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Mar 2, 2019 •

edited

Loading