Prototype code on what might look like to remove the AttributeValue from the API by jkwatson · Pull Request #1500 · open-telemetry/opentelemetry-java

jkwatson · 2020-08-03T23:47:21Z

The goal of this experiment is to figure out how to efficiently write & read strongly-typed attributes into and out of an immutable collection-like structure. This shouldn't be taken as a proposal, but just the result of my noodlings on how to you could build an API like this.

Notes:

Ignore the naming..the names aren't intended to be final..just something I could have live next to the existing interfaces and classes.
I had to give up the of(...) constructs, since you would have no type-safety on the values, and the implementation would have to try to figure out how to divine the types and reject non-conforming values if we wanted to just have of(String, Object) as the style. So, I just kept the builder, instead.
On the reading side of the equation, things are not pretty, since I can only hand out the type and the object, and force the reader to either cast to the right type or use some helper methods to do the cast for them.

Other approaches I considered (in no order...none of them are great):

Add union types to the JLS ( )
Make the AttributeConsumer interface have 8 separate methods on it to get the various typed values out, hence forcing readers to implement all 8 methods in order to read anything.
Drop value-type-safety altogether, forcing consumers to figure it out for themselves
Revisit my strongly-typed key proposal from a few months back..this wouldn't help out the auto-instrumentation code, though, as it would just move the wrapping over to the key.

Here is some sample code that utilizes the hacky API I built here:

public class Main {

  public static void main(String[] args) {
    CleanAttributes attributes =
        CleanAttributes.newBuilder()
            .setString("string", "I'm a String")
            .setBoolean("boolean", true)
            .setDouble("double", 33.444d)
            .setLong("long", 34333L)
            .setStringArray("stringArray", "one", "two", "three")
            .setBooleanArray("booleanArray", true, false, true)
            .setLongArray("longArray", 33L, 55L, 99L)
            .setDoubleArray("doubleArray", 123.33, 6655.33, 339393.33, 3434.33)
            .build();

    System.out.println("attributes = " + attributes);
    System.out.println();
    System.out.println("Processing with casts:");
    processRaw(attributes);

    System.out.println();
    System.out.println("Processing with types:");
    processTyped(attributes);
  }

  public static void processRaw(CleanReadableAttributes attributes) {
    attributes.forEach(
        new RawAttributeConsumer() {
          @SuppressWarnings("unchecked")
          @Override
          public void consume(String key, AttributeType type, Object value) {
            switch (type) {
              case STRING:
                String stringValue = (String) value;
                System.out.println("stringValue = " + stringValue);
                break;
              case BOOLEAN:
                boolean booleanValue = (boolean) value;
                System.out.println("booleanValue = " + booleanValue);
                break;
              case LONG:
                long longValue = (long) value;
                System.out.println("longValue = " + longValue);
                break;
              case DOUBLE:
                double doubleValue = (double) value;
                System.out.println("doubleValue = " + doubleValue);
                break;
              case STRING_ARRAY:
                List<String> stringArrayValue = (List<String>) value;
                System.out.println("stringArrayValue = " + stringArrayValue);
                break;
              case BOOLEAN_ARRAY:
                List<Boolean> booleanArrayValue = (List<Boolean>) value;
                System.out.println("booleanArrayValue = " + booleanArrayValue);
                break;
              case LONG_ARRAY:
                List<Long> longArrayValue = (List<Long>) value;
                System.out.println("longArrayValue = " + longArrayValue);
                break;
              case DOUBLE_ARRAY:
                List<Double> doubleArrayValue = (List<Double>) value;
                System.out.println("doubleArrayValue = " + doubleArrayValue);
                break;
            }
          }
        });
  }

  private static void processTyped(CleanAttributes attributes) {
    attributes.forEach(
        new TypedAttributeConsumer() {
          @Override
          public void consumeString(String key, String value) {
            System.out.println(key + " = " + value);
          }

          @Override
          public void consumeLong(String key, long value) {
            System.out.println(key + " = " + value);
          }

          @Override
          public void consumeDouble(String key, double value) {
            System.out.println(key + " = " + value);
          }

          @Override
          public void consumeBoolean(String key, boolean value) {
            System.out.println(key + " = " + value);
          }

          @Override
          public void consumeStringArray(String key, List<String> value) {
            System.out.println(key + " = " + value);
          }

          @Override
          public void consumeLongArray(String key, List<Long> value) {
            System.out.println(key + " = " + value);
          }

          @Override
          public void consumeDoubleArray(String key, List<Double> value) {
            System.out.println(key + " = " + value);
          }

          @Override
          public void consumeBooleanArray(String key, List<Boolean> value) {
            System.out.println(key + " = " + value);
          }
        });
  }
}

codecov · 2020-08-03T23:59:40Z

Codecov Report

Merging #1500 into master will decrease coverage by 5.24%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1500      +/-   ##
============================================
- Coverage     91.45%   86.21%   -5.25%     
  Complexity      949      949              
============================================
  Files           116      120       +4     
  Lines          3405     3612     +207     
  Branches        281      292      +11     
============================================
  Hits           3114     3114              
- Misses          204      411     +207     
  Partials         87       87

Impacted Files	Coverage Δ	Complexity Δ
...in/java/io/opentelemetry/common/AttributeType.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../java/io/opentelemetry/common/CleanAttributes.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...try/common/HeterogenousImmutableKeyValuePairs.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...pi/src/main/java/io/opentelemetry/common/Main.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`

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 7828283...bfeee75. Read the comment docs.

anuraaga · 2020-08-04T00:40:35Z

This shouldn't be taken as a proposal, but just the result of my noodlings on how to you could build an API like this.

Not sure whether you want feedback or not ;) But if you do, maybe you can temporarily copy in the Main code you have there into the PR? Would be easier to add comments about the API.

jkwatson · 2020-08-04T02:15:32Z

This shouldn't be taken as a proposal, but just the result of my noodlings on how to you could build an API like this.

Not sure whether you want feedback or not ;) But if you do, maybe you can temporarily copy in the Main code you have there into the PR? Would be easier to add comments about the API.

I would love feedback/thoughts/ideas/etc. I added the Main class/method.

anuraaga · 2020-08-04T03:47:03Z

+          public void consume(String key, AttributeValue.Type type, Object value) {
+            switch (type) {
+              case STRING:
+                String stringValue = attributes.getStringValue(value);


This could just be (String) value, there doesn't seem to be much value to delegate the cast.

yeah, I have gone back and forth on this one. I agree with you, but I also like having the option for the non-obvious ones (like the "array" values that are actually Lists). You can definitely just have the consumer do the casts manually, for sure.

Can you clarify "non-obvious"? I guess just that it looks a little gross (which I agree with but not enough to make the indirect API I think)

List<String> stringArrayValue = (List<String>) attributes.getStringArrayValue(value);

That being said, I think requiring casts does have the downside of requiring the suppresswarnings "unchecked" too if those are enabled. I guess if we provide a helper whose purpose is a cast, we could make it static instead of going through the attributes instance.

getStringArrayValue implies String[] not List<String>

How about String AttributeValue.Type.STRING.cast(Object value) and similar for other types? That will clean up the API of Attributes

excellent. something like this has been tickling the back of my brain, but I couldn't quite put my finger on it. I will try this out and add this API to this PR to see how it feels/looks.

hmm. I'm not sure how you get a handle to call those methods. They don't appear to be visible to be called anywhere. Java doesn't let you add static members to enum instances, either, unfortunately.

yeah, you can't call methods like that on enum values. See here for the JEP to make it possible: http://openjdk.java.net/jeps/301

I moved the methods to do the casts into a new AttributeType class, which just moves the issue to a different place. I haven't been able to figure out a way to make it so you can only call the right one, yet.

I also added an 8-way typed consumer interface, just to see how that worked out. With the requirement for a consumer to do the casts with the single method, 8 methods doesn't look too bad. But, also doesn't have the advantage of being a SMI for implementation via a lambda.

anuraaga · 2020-08-04T03:48:10Z

+                Double doubleValue = attributes.getDoubleValue(value);
+                System.out.println("doubleValue = " + doubleValue);
+                break;
+              case STRING_ARRAY:


It's too bad there's no way to do instanceof List<String>, Java 15 would look quite nice

if (value instanceof String) { String foo = value; // Automatically cast; } else if (value instanceof List<String>) // Unfortunately can't :(

agreed. or kotlin. :)

anuraaga · 2020-08-04T03:49:33Z

+  /** Iterates over all the key-value pairs of attributes contained by this instance. */
+  void forEach(AttributeConsumer consumer);
+
+  Boolean getBooleanValue(Object value);


Mentioned it below, but these seem to not provide much value. But boolean getBoolean(String key), now that sounds nice :)

I think we should demand to the users the type retrieval. This way, we do not have to define what should happen if we call getBoolean(key) on an attribute that was stored as long, for example.

I think such methods are exactly demanding users to do the type retrieval - they are explicitly picking the method to call. But it gives us the ability to provide a UX possibly better for users. If we throw an exception, it's no different than any other option. But we could also treat it as an empty value if it makes sense - I'd lean towards this given we generally don't want to crash user's apps. With it in the javadoc it's pretty straightforward.

Yes, this is probably the use-case that seems most beneficial with having these extra helper methods. We can do some intelligent default behavior when the wrong one is called, and make that uniform and documented. And, if people don't want to use them, they can do the cast themselves and handle the bad conditions however they desire.

This is one of the main values of having the typed-keys..having a type-safe API for retrieval, because then you can't ask for a boolean value from a String-typed key.

If we don't use primitive types and we return null value in case of the wrong type, I agree. It would make a better UX for users.

One option I did consider was to have the API not give the consumer the Object value, but require them to make a get call based on the type and the value, which could enforce that the right type is passed in for the right method. This would necessitate, probably, doing some optimization of the get code path, which currently just traverses the entire list to find the key. I wasn't sure how exactly to do that. One option would be to provide an Iterator that kept the index to the currently location in the list, rather than doing it via the forEach and an consumer.

I agree with Anuraag that boolean getBoolean(String key) etc would make a nice API for single key retrieval.

easy to add, but wouldn't help with the iteration use-case, which I think will be the most common one, unfortunately.

anuraaga

Make the AttributeConsumer interface have 8 separate methods on it to get the various typed values out, hence forcing readers to implement all 8 methods in order to read anything.

Realized when looking at the switch again, this option may not be so bad.

anuraaga · 2020-08-05T07:04:47Z

+          public void consume(String key, AttributeValue.Type type, Object value) {
+            switch (type) {
+              case STRING:
+                String stringValue = attributes.getStringValue(value);


Can you clarify "non-obvious"? I guess just that it looks a little gross (which I agree with but not enough to make the indirect API I think)

List<String> stringArrayValue = (List<String>) attributes.getStringArrayValue(value);

That being said, I think requiring casts does have the downside of requiring the suppresswarnings "unchecked" too if those are enabled. I guess if we provide a helper whose purpose is a cast, we could make it static instead of going through the attributes instance.

anuraaga · 2020-08-06T07:10:10Z

+
+  private static void processTyped(CleanAttributes attributes) {
+    attributes.forEach(
+        new TypedAttributeConsumer() {


I like this a bit better than the switch. No noise (asString, etc) just pure consumption

me, too, although implementing a 8-method interface is not super ergonomic, either. Especially in the cases where you don't care about the types, and just want to gather raw data [if this is a use-case?]

Easy :) Copy all attributes to another Attributes before you add your own attributes to it :)

My problem is the opposite: I cannot imagine when do you to iterate over all attributes in a type-safe manner.

any exporter needs to do this. See all the exporters in the repo that have to do this work.

@jkwatson Maybe we should knock out Attributes.toBuilder now since looks like @iNikem really wants it :P

I think that's great to have, no matter what, so yes, definitely.

anuraaga · 2020-08-06T07:12:39Z

+          public void consume(String key, AttributeType type, Object value) {
+            switch (type) {
+              case STRING:
+                String stringValue = type.asString(value);


Not strong but I might find this a bit more readable

enum AttributeType { STRING, BOOLEAN, FOO, CAT; @SuppressWarnings("unchecked") public <T> T cast(Object dog) { return (T) dog; } case STRING: String stringValue = STRING.cast(value); // Having the same STRING seems easy to read

I still like my original proposal more :) This code and Anuraag's suggestion are equal to me.

I don't know how to implement your proposal. Can you add a PR to this one to show me?

@anuraaga it's more readable, for sure, but 100% not type-safe. The user can do the wrong thing trivially, which seems to defeat the purpose, to me. I have this locally, though, and will commit it. :)

Is it easier to do the wrong thing with that vs type.asString? They still need to call the correct method in either case I think, but using the same STRING twice is more obvious I think for a reader of the code since there's symmetry there.

Well, using the type capture, you could put any old value on the LHS and it would still compile (BigDecimal, SpanContext, you name it). So, with the type.as... methods, at least they only have a limited number of options?

Unfortunately, at this point we just discussing the least bad option, I'll admit. Java's type system just doesn't have the facilities to make a really good API, without going back to AttributeValue, as far as I can see.

I could probably get behind any of these options, TBH. I think the least-bad option is the 8-way interface, because at least in that case they're forced to consider all the possible types. We can also have the single one and require the manual casting along with that, so at least both options are available, depending on the use-case.

+1 vote to 8-way interface (and optionally also single casting variant, but with toBuilder the latter becomes less important perhaps)

jkwatson · 2020-08-11T23:52:02Z

I've pruned this down to 2 options: 1) The 8-way typed consumer interface, and 2) the completely open consumer interface.

I'd really like to make a decision on weather we want to go down this route. If we do, I'll prep a PR that converts the repo over to this. @bogdandrutu @carlosalberto Your opinions would be very useful here.

anuraaga

My vote is to add both APIs, they LGTM

…ollection of strongly typed attributes.

… essentials. Also added a toBuilder to the CleanAttributes since it's been added to the Attributes class.

jkwatson · 2020-09-10T19:58:04Z

closing this, as I've now got an actual PR submitted that I think most of us like.

anuraaga reviewed Aug 4, 2020

View reviewed changes

anuraaga reviewed Aug 5, 2020

View reviewed changes

anuraaga reviewed Aug 6, 2020

View reviewed changes

jkwatson force-pushed the attribute_value_removal_hacking branch from fce23c7 to 000b710 Compare August 11, 2020 23:46

jkwatson changed the title ~~Hacky, prototypish code on what might look like to remove the AttributeValue from the API~~ Prototype code on what might look like to remove the AttributeValue from the API Aug 11, 2020

anuraaga reviewed Aug 12, 2020

View reviewed changes

jwatson added 7 commits August 13, 2020 14:10

Some noodling on how to efficiently query a heterogenous, immutable c…

ced19e7

…ollection of strongly typed attributes.

Add the main method that shows how the API works, or doesn't

6cd9238

move the casting to the type itself, make set names have type in them

1d39407

added a consumer with all 8 types on it

a1e2309

Add the magic type-capture casting option, for comparison

1164646

Add the warning about the type capture usage

906c8f6

remove the unpopular parts of the experiment, and trim it down to the…

bfeee75

… essentials. Also added a toBuilder to the CleanAttributes since it's been added to the Attributes class.

jkwatson force-pushed the attribute_value_removal_hacking branch from 000b710 to bfeee75 Compare August 13, 2020 21:14

This was referenced Aug 23, 2020

Proposal: Remove AttributeValue class and implement attribute value type-safety with a type-indicating Key. #1580

Closed

Convert Attributes and AttributeValue into interfaces #1589

Closed

jkwatson closed this Sep 10, 2020

Conversation

jkwatson commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anuraaga commented Aug 4, 2020

Uh oh!

jkwatson commented Aug 4, 2020

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!

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!

anuraaga 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!

Choose a reason for hiding this comment

Uh oh!

jkwatson commented Aug 3, 2020 •

edited

Loading

codecov bot commented Aug 3, 2020 •

edited

Loading