Deprecate MonitoredResource and change to use Resource. by bogdandrutu · Pull Request #1718 · census-instrumentation/opencensus-java

bogdandrutu · 2019-01-30T02:12:25Z

No description provided.

mayurkale22 · 2019-01-30T02:15:56Z

Can we combine this with #1590?

bogdandrutu · 2019-01-30T02:19:01Z

@mayurkale22 feel free to push a PR after this 👍 (seriously already large changes, I was actually trying to understand how to split, put I think is simple enough)

songy23 · 2019-01-30T04:22:50Z

Please fix the nullness checker.

codecov-io · 2019-01-30T05:23:03Z

Codecov Report

Merging #1718 into master will increase coverage by 0.18%.
The diff coverage is 88.63%.

@@             Coverage Diff              @@
##             master    #1718      +/-   ##
============================================
+ Coverage     81.99%   82.17%   +0.18%     
- Complexity     1904     1909       +5     
============================================
  Files           280      283       +3     
  Lines          8937     8956      +19     
  Branches        850      849       -1     
============================================
+ Hits           7328     7360      +32     
+ Misses         1327     1316      -11     
+ Partials        282      280       -2

Impacted Files	Coverage Δ	Complexity Δ
...race/stackdriver/StackdriverV2ExporterHandler.java	`86.2% <100%> (+0.05%)`	`42 <0> (ø)`	⬇️
...s/contrib/monitoredresource/util/ResourceType.java	`100% <100%> (ø)`	`1 <1> (ø)`	⬇️
...rter/stats/stackdriver/StackdriverExportUtils.java	`90.74% <100%> (+0.03%)`	`57 <0> (ø)`	⬇️
...monitoredresource/util/GcpGceInstanceResource.java	`100% <100%> (ø)`	`3 <3> (?)`
...monitoredresource/util/MonitoredResourceUtils.java	`31.25% <28.57%> (ø)`	`2 <0> (ø)`	⬇️
...trib/monitoredresource/util/MonitoredResource.java	`92.1% <90.32%> (+24.18%)`	`1 <0> (-2)`	⬇️
...b/monitoredresource/util/K8sContainerResource.java	`90.9% <90.9%> (ø)`	`2 <2> (?)`
...monitoredresource/util/AwsEc2InstanceResource.java	`91.66% <91.66%> (ø)`	`2 <2> (?)`
... and 2 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 e8cba95...53cf379. Read the comment docs.

bogdandrutu · 2019-01-30T05:29:42Z

@songy23 done

songy23 · 2019-01-30T19:03:31Z

Please fix the import.

bogdandrutu · 2019-01-30T22:58:36Z

@songy23 finally working.

songy23

LGTM except minor comments.

songy23 · 2019-01-30T23:04:49Z

     */
-    public abstract String getAccount();
+    public String getAccount() {
+      return firstNonNull(awsInstanceLabels.get(AwsEc2InstanceResource.ACCOUNT_ID_KEY), "");


`firstNonNull here is not needed because it's already called: https://github.com/census-instrumentation/opencensus-java/pull/1718/files#diff-e0f6d1e015f871479d844cca09da8bc7R34. Same for all others in this class.

Ignore this comment - just saw this is the key not the value.

songy23 · 2019-01-30T23:06:38Z

+ *
+ * @since 0.20
+ */
+public class K8sContainerResource {


So project, instance-id and zone do not apply to the k8s resource?

They are not related to k8s resource, but when we do autodetection if the pod/container runs on a GCE instance they will be added in the resource merging.

OK, make sense.

songy23 · 2019-01-30T23:34:43Z

+ *
+ * @since 0.20
+ */
+public class K8sContainerResource {


OK, make sense.

bogdandrutu requested review from mayurkale22 and songy23 January 30, 2019 02:12

googlebot added the cla: yes label Jan 30, 2019

bogdandrutu force-pushed the monitored branch from bb47567 to 0140612 Compare January 30, 2019 05:23

bogdandrutu force-pushed the monitored branch from 0140612 to 6afe46f Compare January 30, 2019 05:45

bogdandrutu added 4 commits January 30, 2019 14:24

Deprecate MonitoredResource and change to use Resource.

890f141

Fix deprecated usages and imports.

a53aa5b

Resolve more deprecation usage warnings.

91f8e45

Use full class names for deprecated classes

53cf379

bogdandrutu force-pushed the monitored branch from 6afe46f to 53cf379 Compare January 30, 2019 22:28

mayurkale22 approved these changes Jan 30, 2019

View reviewed changes

songy23 reviewed Jan 30, 2019

View reviewed changes

songy23 approved these changes Jan 30, 2019

View reviewed changes

bogdandrutu merged commit 1cd62e4 into census-instrumentation:master Jan 30, 2019

bogdandrutu deleted the monitored branch January 30, 2019 23:47

c24t mentioned this pull request Feb 20, 2019

Replace MonitoredResource with Resource census-instrumentation/opencensus-python#508

Merged

Conversation

bogdandrutu commented Jan 30, 2019

Uh oh!

mayurkale22 commented Jan 30, 2019

Uh oh!

bogdandrutu commented Jan 30, 2019

Uh oh!

songy23 commented Jan 30, 2019

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

bogdandrutu commented Jan 30, 2019

Uh oh!

songy23 commented Jan 30, 2019

Uh oh!

bogdandrutu commented Jan 30, 2019

Uh oh!

songy23 left a comment

Choose a reason for hiding this comment

Uh oh!

songy23 Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

songy23 Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

songy23 Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

bogdandrutu Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

songy23 Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

songy23 Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jan 30, 2019 •

edited

Loading