Stats/Stackdriver: export data using metrics API by mayurkale22 · Pull Request #1501 · census-instrumentation/opencensus-java

mayurkale22 · 2018-10-17T05:30:55Z

Dependency on #1499

This PR is part of #1483

codecov-io · 2018-10-17T05:50:55Z

Codecov Report

Merging #1501 into master will decrease coverage by 0.3%.
The diff coverage is 94.73%.

@@             Coverage Diff              @@
##             master    #1501      +/-   ##
============================================
- Coverage      82.6%   82.29%   -0.31%     
+ Complexity     1648     1628      -20     
============================================
  Files           251      247       -4     
  Lines          7782     7576     -206     
  Branches        743      728      -15     
============================================
- Hits           6428     6235     -193     
+ Misses         1129     1117      -12     
+ Partials        225      224       -1

Impacted Files	Coverage Δ	Complexity Δ
...er/stats/stackdriver/StackdriverStatsExporter.java	`65.27% <100%> (ø)`	`11 <0> (ø)`	⬇️
...r/stats/stackdriver/StackdriverExporterWorker.java	`72.26% <94.11%> (+0.23%)`	`19 <9> (ø)`	⬇️
...rter/stats/stackdriver/StackdriverExportUtils.java	`78.23% <94.8%> (-4.78%)`	`37 <20> (+6)`
...pencensus/implcore/metrics/MetricRegistryImpl.java	`92.1% <0%> (-6.26%)`	`3% <0%> (-3%)`
.../io/opencensus/implcore/metrics/LongGaugeImpl.java	`93.33% <0%> (-0.22%)`	`15% <0%> (ø)`
...o/opencensus/implcore/metrics/DoubleGaugeImpl.java	`93.33% <0%> (-0.22%)`	`15% <0%> (ø)`
.../java/io/opencensus/metrics/export/TimeSeries.java	`100% <0%> (ø)`	`4% <0%> (ø)`	⬇️
...io/opencensus/metrics/export/MetricDescriptor.java	`100% <0%> (ø)`	`2% <0%> (ø)`	⬇️
...ain/java/io/opencensus/metrics/MetricRegistry.java	`100% <0%> (ø)`	`2% <0%> (ø)`	⬇️
...src/main/java/io/opencensus/metrics/LongGauge.java	`72.22% <0%> (ø)`	`2% <0%> (ø)`	⬇️
... and 6 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 9c9c152...998d053. Read the comment docs.

songy23 · 2018-10-17T16:09:48Z

buildscripts/import-control.xml

@@ -148,6 +148,7 @@ General guidelines on imports:
    <subpackage name="stats">
      <allow pkg="io.opencensus.stats"/>


Are stats and tags still needed?

Both are still needed for other two exporters (prometheus and signalFx), I will remove that once we update these exporters to use Metrics API.

songy23 · 2018-10-17T16:27:24Z

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

+  // StackDriver.
  static List<TimeSeries> createTimeSeriesList(
-      @javax.annotation.Nullable ViewData viewData,
+      @javax.annotation.Nullable io.opencensus.metrics.export.Metric metric,


The @Nullable annotation is no longer needed.

songy23 · 2018-10-17T16:34:27Z

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

+    return value.match(
+        typedValueDoubleFunction,
+        typedValueLongFunction,
+        new Function<io.opencensus.metrics.export.Distribution, TypedValue>() {


You can make this function static too.

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

...driver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExporterWorker.java

songy23 · 2018-10-17T16:39:30Z

...driver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExporterWorker.java

-    if (existing != null) {
-      if (existing.equals(view)) {
-        // Ignore views that are already registered.
+  boolean registerMetric(Metric metric) {


Same, this should be called registerMetricDescriptor.

songy23 · 2018-10-17T16:40:36Z

...driver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExporterWorker.java

-      if (registerView(view)) {
-        // Only upload stats for valid views.
-        viewDataList.add(viewManager.getView(view.getName()));
+    List</*@Nullable*/ Metric> metricsList = Lists.newArrayList();


@Nullable annotation is no longer needed here, since I don't think metricProducer.getMetrics() would return null.

songy23 · 2018-10-17T16:41:05Z

...driver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExporterWorker.java


    List<TimeSeries> timeSeriesList = Lists.newArrayList();
-    for (/*@Nullable*/ ViewData viewData : viewDataList) {
+    for (/*@Nullable*/ Metric metric : metricsList) {


...river/src/test/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtilsTest.java

songy23 · 2018-10-17T21:07:50Z

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

        .setBucketOptions(createBucketOptions(distribution.getBucketOptions()))
        .addAllBucketCounts(createBucketCounts(distribution.getBuckets()))
        .setCount(distribution.getCount())
+        .setMean(distribution.getSum() / distribution.getCount())


We should check for divided-by-0 here.

mayurkale22 · 2018-10-19T18:15:38Z

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

-    for (int i = 0; i < tagValues.size(); i++) {
-      TagKey key = columns.get(i);
-      TagValue value = tagValues.get(i);
+        labelKeys.size() == labelValues.size(), "LabelKeys and LabelValues don't have same size.");


I see ambiguity here, if someone using the stats API will get confused by seeing this message. Can we say "TagKeys/LabelKeys and TagValues/LabelValues don't have same size." @bogdandrutu @songy23

Maybe it's less confusing to just call them "key" and "value"? i.e "Keys and Values don't have same size."

...driver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExporterWorker.java

mayurkale22 · 2018-10-19T18:46:53Z

Comparison after and before change on same data set. Left side OC16 (SD using Stats API), Right side OC17(SD using Metrics API).

songy23 · 2018-10-22T17:27:48Z

...er/src/test/java/io/opencensus/exporter/stats/stackdriver/StackdriverExporterWorkerTest.java

-        View.create(VIEW_NAME, VIEW_DESCRIPTION, MEASURE, SUM, Arrays.asList(KEY), CUMULATIVE);
-    doReturn(ImmutableSet.of(view)).when(mockViewManager).getAllExportedViews();
+  public void doNotExportIfFailedToRegisterMetric() {
+    doReturn(ImmutableSet.of(metricProducer)).when(metricProducerManager).getAllMetricProducer();


nit: This line can be extracted to

@Before public void setUp() { ... }

...driver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExporterWorker.java

bogdandrutu · 2018-10-24T11:11:36Z

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

+    String type = generateType(metricDescriptor.getName(), domain);
    // Name format refers to
    // cloud.google.com/monitoring/api/ref_v3/rest/v3/projects.metricDescriptors/create
    builder.setName(String.format("projects/%s/metricDescriptors/%s", projectId, type));


I know that you did not write this but is probably faster to do string concatenation.

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

bogdandrutu · 2018-10-24T11:19:07Z

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

-      TagValue value = tagValues.get(i);
+    List<LabelKey> labelKeys = metricDescriptor.getLabelKeys();
+
+    checkArgument(labelKeys.size() == labelValues.size(), "Keys and Values don't have same size.");


We already check for this when we create the Metric so I think this is useless.

...ackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java

googlebot added the cla: yes label Oct 17, 2018

mayurkale22 requested review from bogdandrutu and songy23 October 17, 2018 05:31

songy23 reviewed Oct 17, 2018

View reviewed changes

mayurkale22 commented Oct 19, 2018

View reviewed changes

...driver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExporterWorker.java Show resolved Hide resolved

Stats/Stackdriver: export data using metrics API

031c6f5

songy23 approved these changes Oct 22, 2018

View reviewed changes

Fix review comments

bf815c6

songy23 mentioned this pull request Oct 22, 2018

Update exporter to consume Metrics instead of ViewData. census-ecosystem/opencensus-go-exporter-stackdriver#55

Closed

mayurkale22 added the exporters label Oct 22, 2018

bogdandrutu reviewed Oct 24, 2018

View reviewed changes

Fix review comments

998d053

bogdandrutu approved these changes Oct 25, 2018

View reviewed changes

mayurkale22 merged commit cd62646 into census-instrumentation:master Oct 25, 2018

mayurkale22 deleted the sd_metrics branch October 25, 2018 15:15

mayurkale22 mentioned this pull request Dec 28, 2018

Rewrite stats exporters to use Metrics instead of Stats (ViewData) census-instrumentation/opencensus-node#257

Open

2 tasks

c24t mentioned this pull request Jan 17, 2019

Refactor stats exporters to use metrics data model census-instrumentation/opencensus-python#454

Open

		@@ -148,6 +148,7 @@ General guidelines on imports:
		<subpackage name="stats">
		<allow pkg="io.opencensus.stats"/>

Conversation

mayurkale22 commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 17, 2018 • 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!

Uh oh!

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!

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!

Uh oh!

mayurkale22 commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mayurkale22 commented Oct 17, 2018 •

edited

Loading

codecov-io commented Oct 17, 2018 •

edited

Loading