Work in Progress on some Trace API tweaks by jkwatson · Pull Request #1130 · open-telemetry/opentelemetry-java

jkwatson · 2020-04-22T17:42:49Z

Doing this in Draft PR to make it as open and public as possible.

Also, doing this via a new contrib module for incubating new APIs.

codecov-io · 2020-04-22T17:51:26Z

Codecov Report

Merging #1130 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1130   +/-   ##
=========================================
  Coverage     84.17%   84.17%           
  Complexity     1153     1153           
=========================================
  Files           149      149           
  Lines          4449     4449           
  Branches        394      394           
=========================================
  Hits           3745     3745           
  Misses          557      557           
  Partials        147      147

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 a71f905...37c87b9. Read the comment docs.

thisthat · 2020-04-27T07:17:34Z

+  private static Map<String, AttributeValue> makeAttributeMap(Attribute[] attributes) {
+    Map<String, AttributeValue> result = new HashMap<>();
+    for (Attribute attribute : attributes) {
+      result.put(attribute.key().key(), makeValue(attribute));


Having this double key method call looks strange.
The fact that we need to unwrap and wrap again the value makes me thinking that maybe the concrete implementation of Attributes should also already build the proper AttributeValue and hold its reference. This way we don't need a switch since the typing information already suggests the correct method to call. WDYT?

The goal of this draft PR is to discuss changes to the API, not provide an efficient wrapper for the current API. Don't worry about the internal details of this, as they are just there to make sure we can provide the same functionality, not to be a real implementation.

Remember, the impetus for this discussion was getting rid of AttributeValue from the API altogether. :)

thisthat · 2020-04-27T07:18:24Z

+import com.google.errorprone.annotations.MustBeClosed;
+import io.opentelemetry.context.Scope;
+
+// go Speed Racer, go Speed Racer, go Speed Racer go!


Oberon00 · 2020-04-27T11:37:29Z

Am I missing something or is there not yet a SpanEx.put/setAttribute outside the Builder?

jkwatson · 2020-04-27T15:15:09Z

Am I missing something or is there not yet a SpanEx.put/setAttribute outside the Builder?

I've only written options for the Link API at this point. If we can decide on a direction for that, I think the rest will follow pretty clearly.

jkwatson · 2020-04-27T18:57:08Z

@bogdandrutu I added some detailed notes about allocation overhead with some of the various options in this PR.

jkwatson · 2020-05-01T14:59:37Z

@Oberon00 @thisthat @trask Please provide feedback on the various options presented here.

trask

i think i prefer the more efficient and more user-friendly setAttribute(key, value).

from a user perspective, i don't think the overloaded methods make the API surface feel bigger (they only add to the API surface from a maintainer's perspective).

trask · 2020-05-16T01:08:10Z

+    // option 1... provide a single attribute
+    public SpanEx.Builder setAttribute(Attribute attribute) {
+      builder.setAttribute(attribute.key().key(), makeValue(attribute));
+      return this;
+    }


comparing

spanBuilder.setAttribute(Attribute.create(key, value))

and

spanBuilder.setAttribute(key, value)

the latter is more efficient, and seems more user-friendly to me

ah, cool. This is the obvious option that we should add. I assume you mean with the typed-key. Yes, I'll add this ASAP.

I added this, as well as all the array-based attribute methods everywhere, for completeness.

trask · 2020-05-16T01:13:29Z

+    public SpanEx.Builder setAttribute(Attribute... attributes) {
+      for (Attribute attribute : attributes) {
+        builder.setAttribute(attribute.key().key(), makeValue(attribute));
+      }
+      return this;
+    }


comparing

Attribute attr1 = Attribute.create(key1, value1); Attribute attr2 = Attribute.create(key2, value2); Attribute attr3 = Attribute.create(key3, value3); spanBuilder.setAttribute(attr1, attr2, attr3);

and

spanBuilder.setAttribute(key1, value1) .setAttribute(key2, value2) .setAttribute(key3, value3);

the latter is more efficient, and seems more user-friendly to me

check out the latest updates, which have support for this usage.

trask · 2020-05-16T01:31:59Z

+    // option 1... provide a single attribute
+    public SpanEx.Builder setAttribute(Attribute attribute) {
+      builder.setAttribute(attribute.key().key(), makeValue(attribute));
+      return this;
+    }
+
+    // option 2... a type-safe key-value pair with varargs parameters
+
+    /** doc me. */
+    public SpanEx.Builder setAttribute(Attribute... attributes) {
+      for (Attribute attribute : attributes) {
+        builder.setAttribute(attribute.key().key(), makeValue(attribute));
+      }
+      return this;
+    }
+
+    // option 3... accept a bunch of attributes at once
+
+    /** doc me. */
+    public SpanEx.Builder setAttribute(Attributes attributes) {
+      for (AttributeKey key : attributes.getKeys()) {
+        builder.setAttribute(key.key(), makeValue(attributes, key));
+      }
+      return this;
+    }


comparing

spanBuilder.setAttributes( MapBasedAttributes.newBuilder().put(key1, value1) .put(key2, value2) .put(key3, value3) .build())

and

spanBuilder.setAttribute(key1, value1) .setAttribute(key2, value2) .setAttribute(key3, value3);

the latter is more efficient, and seems more user-friendly to me

i think i prefer the more efficient and more user-friendly setAttribute(key, value).

from a user perspective, i don't think the overloaded methods make the API surface feel bigger (they only add to the API surface from a maintainer's perspective).

I agree with you that we should offer the setAttribute(key, value) but I think we should also offer the 3rd option using Attributes. In many semantic conventions, say HTTP, many attributes don't change. For example, if a library has to manually instrument its code to serve an HTTP request, the attributes method, url, target, ... do not change between requests so they can be cached.

This is an interesting thought, yes. Pre-bundling the attributes that are relevant for a given HTTP endpoint is possible, but is that likely to be a common use-case, as opposed to more generic instrumentation that would need to dynamically generate the values?

Would caching an Attributes object give some performance benefit? Or is it just for convenience? Maybe a concrete example would help to understand how caching the Attributes would be more convenient compared to settings the attributes directly.

I was thinking out loud without having a concrete example in mind. Thinking more thoroughly on a possible usage, I would use the Attributes interface to decouple span creation and attribute definition, similarly to how it's used here:

https://github.com/newrelic-forks/opentelemetry-java/blob/37c87b907289de35be6dfe4c3aee132e1815b7db/contrib/api_incubator/src/test/java/io/opentelemetry/trace/TracerExTest.java#L84-L85

The decoupling is nice in that example, but if the attribute values aren't static, I think you'd need to pass in 8 parameters to that httpAttribute() method, at which point it may not be so nice.

…ental tracer/span API.

…per-entry allocations

…n for Link attributes.

jkwatson · 2020-06-30T14:59:20Z

I'm going to close this, since the introduction of the Attributes class would mean a rework, if we wanted to change to "typed" keys.

jkwatson mentioned this pull request Apr 22, 2020

Proposal: Remove AttributeValue from the public interface. #1081

Closed

jkwatson mentioned this pull request Apr 24, 2020

Prototype of removing bare span attribute keys #1097

Closed

jkwatson force-pushed the experimental_tracer_api branch from 60940a0 to a9aef48 Compare April 24, 2020 20:51

thisthat reviewed Apr 27, 2020

View reviewed changes

Oberon00 mentioned this pull request Apr 27, 2020

SemanticAttribute setters are not straightforward to use with SpanBuilder #1075

Closed

jkwatson force-pushed the experimental_tracer_api branch from dc14421 to 82e10c0 Compare April 27, 2020 16:52

jkwatson mentioned this pull request Apr 29, 2020

Prototype of making applying semantic attributes a little more type-safe #1096

Closed

trask reviewed May 16, 2020

View reviewed changes

jkwatson force-pushed the experimental_tracer_api branch from de62548 to 28640f4 Compare May 18, 2020 20:50

jwatson and others added 13 commits May 18, 2020 13:52

Introduce the api incubator and create the first parts of the experim…

d693bff

…ental tracer/span API.

Add in the basic, non-attribute related API delegates

2c26f06

two options for adding links to a span builder

e45658b

provide a third Link option, with a concrete class

77d1cb3

move a comment around

33cf9d0

move the type-specific keys into the interface as inner classes

b1bbfbb

update to java 8, and make it simpler to create the keys

020d67f

add a warning suppressor

c90894c

Write an array-based Attributes implementation and add commentary on …

4db9aff

…per-entry allocations

add a little clarification

33b5dc6

Add comparison allocation analysis for the existing API implementatio…

801b541

…n for Link attributes.

Playing with the API

7ab5463

Add more methods and usages of the experimental attributes API.

37c87b9

jkwatson force-pushed the experimental_tracer_api branch from 28640f4 to 37c87b9 Compare May 18, 2020 20:53

jkwatson mentioned this pull request May 21, 2020

Change public APIs to not accept Map<String, AttributeValue> #1258

Closed

jkwatson mentioned this pull request Jun 3, 2020

Create an Attributes immutable container to replace Map<String, AttributeValue> #1278

Closed

3 tasks

jkwatson closed this Jun 30, 2020

Conversation

jkwatson commented Apr 22, 2020

Uh oh!

codecov-io commented Apr 22, 2020 • 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!

Oberon00 commented Apr 27, 2020

Uh oh!

jkwatson commented Apr 27, 2020

Uh oh!

jkwatson commented Apr 27, 2020

Uh oh!

jkwatson commented May 1, 2020

Uh oh!

trask left a comment

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!

jkwatson commented Jun 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Apr 22, 2020 •

edited

Loading