Stackdriver Trace Exporter: add resource labels to Spans. by songy23 · Pull Request #1243 · census-instrumentation/opencensus-java

songy23 · 2018-06-07T22:18:35Z

Resource labels are consistent with Specs. The format of label key is g.co/r/<resource_type>/<label>, for example g.co/r/aws_ec2_instance/aws_account.

codecov-io · 2018-06-07T22:46:57Z

Codecov Report

Merging #1243 into master will decrease coverage by 0.33%.
The diff coverage is 44.64%.

@@             Coverage Diff              @@
##             master    #1243      +/-   ##
============================================
- Coverage     82.06%   81.72%   -0.34%     
- Complexity     1241     1249       +8     
============================================
  Files           193      193              
  Lines          6044     6097      +53     
  Branches        560      562       +2     
============================================
+ Hits           4960     4983      +23     
- Misses          935      962      +27     
- Partials        149      152       +3

Impacted Files	Coverage Δ	Complexity Δ
...race/stackdriver/StackdriverV2ExporterHandler.java	`79.32% <44.64%> (-12.29%)`	`40 <7> (+8)`
...rter/stats/stackdriver/StackdriverExportUtils.java	`81.53% <0%> (ø)`	`29% <0%> (ø)`	⬇️

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 a6e0485...f1e2bdb. Read the comment docs.

sebright2 · 2018-06-08T00:48:09Z

    compileOnly libraries.auto_value

    compile project(':opencensus-api'),
+            project(':opencensus-contrib-monitored-resource-util'),


Is there any issue with adding a dependency from an exporter to a package under contrib/ (for example, if the contrib packages are less stable)?

/cc @bogdandrutu

I cannot think of any particular issue. Stackdriver Stats exporter also depends on this artifact, and I think we need to maintain the same backwards compatibility for contrib/ artifacts to apis.

sebright2 · 2018-06-08T00:56:42Z

    return attributesBuilder;
  }

+  @SuppressWarnings("unchecked")


Why do we need to suppress warnings in this method? Is it possible to suppress them in a smaller scope?

Because of the type casting. I moved it to the casting statement instead.

I'm not sure I understand why the @SuppressWarnings is needed on those lines. They are casting to non-generic types, so the casts are checked.

You're right, removed.

sebright2 · 2018-06-08T01:04:08Z

+            (AwsEc2InstanceMonitoredResource) resource;
+        attributesBuilder.putAttributeMap(
+            createResourceLabelKey(resourceType, "aws_account"),
+            toStringAttributeValueProto(awsEc2InstanceMonitoredResource.getAccount()));


attributesBuilder.putAttributeMap( createResourceLabelKey(resourceType, "aws_account"), toStringAttributeValueProto(awsEc2InstanceMonitoredResource.getAccount()));

Suggestion: The code that adds an attribute to the builder could probably be factored out into a helper method.

Sounds good, done.

sebright2 · 2018-06-08T01:37:51Z

+      return;
+    }
+    ResourceType resourceType = StackdriverV2ExporterHandler.RESOURCE.getResourceType();
+    switch (resourceType) {


It looks like it is possible for this method to not test the logic of putResourceLabels if the ResourceType isn't detected. Is there a way to refactor the code so that the ResourceType is passed in to generateSpan, or putResourceLabels is tested directly? Then we could easily test the code for all three cases, independent of the environment.

Done. The constructor of MonitoredResource is not public so I used some mocks here.

bogdandrutu · 2018-06-08T22:06:09Z

+      String attributeName,
+      String attributeValue) {
+    attributesBuilder.putAttributeMap(
+        createResourceLabelKey(resourceType, attributeName),


We should cache all of these work.
private static final java.util.Map<java.lang.String, com.google.devtools.cloudtrace.v2.AttributeValue> GCE_INSTANCE_LABELS;

attributesBuilder.putAllAttributeMap(GCE_INSTANCE_LABELS);

Make sense to me, though making static map makes it difficult to test.

If we really want to test the resource labels under all 3 cases, we may need to use something like PowerMock to mock MonitoredResourceUtils.getDefaultResource(). @sebright any idea on this?

If it is better to cache the whole map, couldn't we just pass RESOURCE_LABELS into generateSpan, instead of passing in RESOURCE?

Ahh you're right, updated. I also left a TODO to test the parsing.

sebright2 · 2018-06-11T23:38:51Z

    return attributesBuilder;
  }

+  @SuppressWarnings("unchecked")


I'm not sure I understand why the @SuppressWarnings is needed on those lines. They are casting to non-generic types, so the casts are checked.

sebright2 · 2018-06-11T23:57:21Z

+    Map<String, AttributeValue> attributeMap = span.getAttributes().getAttributeMapMap();
+    assertThat(attributeMap).containsKey(createResourceLabelKey(AWS_EC2_INSTANCE, "aws_account"));
+    assertThat(attributeMap).containsKey(createResourceLabelKey(AWS_EC2_INSTANCE, "instance_id"));
+    assertThat(attributeMap).containsKey(createResourceLabelKey(AWS_EC2_INSTANCE, "region"));


Should the three new tests also check the values associated with the resource label keys?

Good catch, done.

sebright2 · 2018-06-12T00:11:32Z

+      String attributeName,
+      String attributeValue) {
+    attributesBuilder.putAttributeMap(
+        createResourceLabelKey(resourceType, attributeName),


If it is better to cache the whole map, couldn't we just pass RESOURCE_LABELS into generateSpan, instead of passing in RESOURCE?

sebright2 · 2018-06-12T02:17:47Z

  }

+  @Test
+  public void generateSpan_WithResourceLabels() {


This change looks good, though I think we should also test the other two ResourceTypes, as in 57765e8.

Done. All these tests tested the same code path though, I'll update this to test parsing MonitoredResource instead, after #1250 is merged.

Stackdriver Trace Exporter: add resource labels to Spans.

0696e3b

songy23 added tracing exporters labels Jun 7, 2018

songy23 assigned bogdandrutu and sebright2 Jun 7, 2018

songy23 requested review from bogdandrutu and sebright2 June 7, 2018 22:18

googlebot added the cla: yes label Jun 7, 2018

songy23 added this to the Release 0.15.0 milestone Jun 7, 2018

songy23 requested a review from rghetia June 7, 2018 22:24

songy23 assigned rghetia Jun 7, 2018

sebright2 reviewed Jun 8, 2018

View reviewed changes

sebright2 assigned songy23 and unassigned sebright2 Jun 8, 2018

Fix review comments.

57765e8

songy23 assigned sebright2 and unassigned songy23 Jun 8, 2018

bogdandrutu reviewed Jun 8, 2018

View reviewed changes

Make resource labels static and only initialize once.

73c9f5e

songy23 force-pushed the sd-trace-resources branch from 73c9f5e to cd76f7e Compare June 8, 2018 23:55

songy23 requested review from bogdandrutu and removed request for bogdandrutu June 11, 2018 16:42

songy23 unassigned bogdandrutu Jun 11, 2018

sebright2 reviewed Jun 12, 2018

View reviewed changes

sebright2 assigned songy23 and unassigned sebright2 Jun 12, 2018

songy23 added 2 commits June 11, 2018 17:57

Remove SuppressWarnings("unchecked")

d62d5ba

Passing resource_label map to generateSpan().

64abe59

songy23 force-pushed the sd-trace-resources branch from cd76f7e to 64abe59 Compare June 12, 2018 01:31

songy23 assigned rghetia and sebright2 and unassigned rghetia and songy23 Jun 12, 2018

Fix a checkstyle error.

55c13af

sebright2 reviewed Jun 12, 2018

View reviewed changes

Add more unit tests on resource labels.

f1e2bdb

sebright2 approved these changes Jun 12, 2018

View reviewed changes

songy23 unassigned sebright2 and rghetia Jun 12, 2018

songy23 merged commit 5926a85 into census-instrumentation:master Jun 12, 2018

songy23 deleted the sd-trace-resources branch June 15, 2018 23:48

Conversation

songy23 commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jun 7, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

songy23 commented Jun 7, 2018 •

edited

Loading

codecov-io commented Jun 7, 2018 •

edited

Loading