Add `Builder` of `Attributes`. Documentation improvements #382

iRevive · 2023-11-21T10:17:53Z

Closes #371.

NthPortal · 2023-11-21T10:38:10Z

wasn't the purpose of the builder to be mutable? while this is perhaps a little more efficient than before, there's still a decent bit of copying

iRevive · 2023-11-21T11:21:45Z

wasn't the purpose of the builder to be mutable? while this is perhaps a little more efficient than before, there's still a decent bit of copying

You are right, we should use a mutable builder instead.

iRevive · 2023-11-21T11:21:57Z

I've added a mutable builder.

NthPortal · 2023-11-21T11:41:58Z

what if we made Attributes extend immutable.Iterable[Attribute[_]]? (happy to push this up to another PR if desired)

iRevive · 2023-11-21T11:46:06Z

what if we made Attributes extend immutable.Iterable[Attribute[_]]?

interesting idea. Then we can drop the custom foldLeft, forall, etc.

iRevive · 2023-11-21T12:16:11Z

@NthPortal thanks for the idea! I like the outcome

iRevive · 2023-11-21T12:19:31Z

sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala

+
+    def iterator: Iterator[Attribute[_]] = m.values.iterator
+
+    override def toList: List[Attribute[_]] = m.values.toList


The compilation fails without this override in Scala 3:

[error] 177 | private final class MapAttributes( [error] | ^ [error] |class MapAttributes needs to be abstract, since def toList: List[org.typelevel.otel4s.Attribute[?]] in trait Attributes in package org.typelevel.otel4s.sdk is not defined [error] one error found

... what?? that makes no sense.

the only thing I can think of is that Iterable already defines toList, but that doesn't really explain things

... what?? that makes no sense.

the only thing I can think of is that Iterable already defines toList, but that doesn't really explain things

I didn't find a meaningful answer either. ¯\_(ツ)_/¯

I assume Scala 3 compiler handles some inheritance cases differently.

sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala

NthPortal · 2023-11-21T22:17:26Z

sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala

+
+  /** Returns the `Map` representation of the attributes collection.
+    */
+  def toMap: Map[AttributeKey[_], Attribute[_]]


I'm torn between this being toMap vs asMap. asMap has the implication that Attributes is already either like a map (and can be easily wrapped as one) or backed by a map. is that something that we should commit to publicly as its design?

Good question. Collections use to{X}. Technically, Attributes is a collection now.
Across the code base, we only use asMap in TraceState.

Collections use to{X}

except for conversions (i.e. wrappers) between Java and Scala collections, which use as{Java,Scala}

NthPortal · 2023-11-21T22:20:40Z

@iRevive I wrote up my own implementation of Attributes as a collection (see #384) which is a bit more complete and has more overrides, as I have lot of experience with the collections framework. however, you added a bunch of docs, which mine does not have. feel free to steal my work and just use it here if you want

sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala

iRevive · 2023-11-22T09:11:10Z

I've backported some changes from #384.

And a few benchmarks:

Benchmark                                    Mode  Cnt      Score    Error   Units
AttributesBenchmark.java_ofOne               avgt   15      4.031 ±  0.008   ns/op
AttributesBenchmark.java_ofFive              avgt   15    134.074 ±  0.086   ns/op
AttributesBenchmark.java_builderTenItem      avgt   15    486.882 ± 13.295   ns/op
AttributesBenchmark.java_computeHashCode     avgt   15     13.207 ±  0.027   ns/op
----------------------------------------------------------------------------------
AttributesBenchmark.otel4s_ofOne             avgt   15     26.083 ±  0.136   ns/op
AttributesBenchmark.otel4s_ofFive            avgt   15    456.209 ±  4.714   ns/op
AttributesBenchmark.otel4s_builderTenItem    avgt   15    792.389 ±  1.734   ns/op
AttributesBenchmark.otel4s_computeHashCode   avgt   15   3018.399 ± 16.982   ns/op

Some overhead is expected because we use Attribute(key, value) under the hood. Java SDK stores keys and values directly in the array.

Benchmarks with GC

Benchmark                                                      Mode  Cnt      Score    Error   Units
AttributesBenchmark.java_builderTenItem                        avgt   15    486.882 ± 13.295   ns/op
AttributesBenchmark.java_builderTenItem:gc.alloc.rate          avgt   15   1081.645 ± 27.242  MB/sec
AttributesBenchmark.java_builderTenItem:gc.alloc.rate.norm     avgt   15    552.001 ±  0.001    B/op
AttributesBenchmark.java_builderTenItem:gc.count               avgt   15     55.000           counts
AttributesBenchmark.java_builderTenItem:gc.time                avgt   15     30.000               ms

AttributesBenchmark.java_computeHashCode                       avgt   15     13.207 ±  0.027   ns/op
AttributesBenchmark.java_computeHashCode:gc.alloc.rate         avgt   15      0.001 ±  0.003  MB/sec
AttributesBenchmark.java_computeHashCode:gc.alloc.rate.norm    avgt   15     ≈ 10⁻⁵             B/op
AttributesBenchmark.java_computeHashCode:gc.count              avgt   15        ≈ 0           counts

AttributesBenchmark.java_ofFive                                avgt   15    134.074 ±  0.086   ns/op
AttributesBenchmark.java_ofFive:gc.alloc.rate                  avgt   15    967.157 ±  0.611  MB/sec
AttributesBenchmark.java_ofFive:gc.alloc.rate.norm             avgt   15    136.000 ±  0.001    B/op
AttributesBenchmark.java_ofFive:gc.count                       avgt   15     48.000           counts
AttributesBenchmark.java_ofFive:gc.time                        avgt   15     25.000               ms

AttributesBenchmark.java_ofOne                                 avgt   15      4.031 ±  0.008   ns/op
AttributesBenchmark.java_ofOne:gc.alloc.rate                   avgt   15  11354.241 ± 23.128  MB/sec
AttributesBenchmark.java_ofOne:gc.alloc.rate.norm              avgt   15     48.000 ±  0.001    B/op
AttributesBenchmark.java_ofOne:gc.count                        avgt   15    319.000           counts
AttributesBenchmark.java_ofOne:gc.time                         avgt   15    171.000               ms

AttributesBenchmark.otel4s_builderTenItem                      avgt   15    792.389 ±  1.734   ns/op
AttributesBenchmark.otel4s_builderTenItem:gc.alloc.rate        avgt   15   3494.463 ±  7.684  MB/sec
AttributesBenchmark.otel4s_builderTenItem:gc.alloc.rate.norm   avgt   15   2904.001 ±  0.002    B/op
AttributesBenchmark.otel4s_builderTenItem:gc.count             avgt   15    175.000           counts
AttributesBenchmark.otel4s_builderTenItem:gc.time              avgt   15     85.000               ms

AttributesBenchmark.otel4s_computeHashCode                     avgt   15   3018.399 ± 16.982   ns/op
AttributesBenchmark.otel4s_computeHashCode:gc.alloc.rate       avgt   15   2325.055 ± 13.112  MB/sec
AttributesBenchmark.otel4s_computeHashCode:gc.alloc.rate.norm  avgt   15   7360.004 ±  0.009    B/op
AttributesBenchmark.otel4s_computeHashCode:gc.count            avgt   15    117.000           counts
AttributesBenchmark.otel4s_computeHashCode:gc.time             avgt   15     59.000               ms

AttributesBenchmark.otel4s_ofFive                              avgt   15    456.209 ±  4.714   ns/op
AttributesBenchmark.otel4s_ofFive:gc.alloc.rate                avgt   15   3394.497 ± 34.863  MB/sec
AttributesBenchmark.otel4s_ofFive:gc.alloc.rate.norm           avgt   15   1624.001 ±  0.001    B/op
AttributesBenchmark.otel4s_ofFive:gc.count                     avgt   15    170.000           counts
AttributesBenchmark.otel4s_ofFive:gc.time                      avgt   15     87.000               ms

AttributesBenchmark.otel4s_ofOne                               avgt   15     26.083 ±  0.136   ns/op
AttributesBenchmark.otel4s_ofOne:gc.alloc.rate                 avgt   15   5264.220 ± 27.369  MB/sec
AttributesBenchmark.otel4s_ofOne:gc.alloc.rate.norm            avgt   15    144.000 ±  0.001    B/op
AttributesBenchmark.otel4s_ofOne:gc.count                      avgt   15    216.000           counts
AttributesBenchmark.otel4s_ofOne:gc.time                       avgt   15    108.000               ms

NthPortal · 2023-11-22T10:48:15Z

Benchmark                                    Mode  Cnt      Score    Error   Units
AttributesBenchmark.java_computeHashCode     avgt   15     13.207 ±  0.027   ns/op

13ns?! is it just identity hash code in the Java implementation?

iRevive · 2023-11-22T10:51:50Z

Benchmark                                    Mode  Cnt      Score    Error   Units
AttributesBenchmark.java_computeHashCode     avgt   15     13.207 ±  0.027   ns/op

13ns?! is it just identity hash code in the Java implementation?

The implementation is this one: https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/main/java/io/opentelemetry/api/common/ArrayBackedAttributes.java.

And the hash function is defined here https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java#L250

NthPortal · 2023-11-22T10:55:11Z

And the hash function is defined here

oh, it caches it in a field. that'll do it. is that worth doing for us? I'm guessing not

iRevive · 2023-11-22T10:58:10Z

oh, it caches it in a field. that'll do it. is that worth doing for us? I'm guessing not

We can keep everything as is, I believe. And implement caching only if the current version will affect performance so badly (I doubt that's the case).

NthPortal · 2023-11-22T10:58:10Z

benchmarks/src/main/scala/org/typelevel/otel4s/benchmarks/AttributesBenchmark.scala

+  private val values = List.tabulate(10)(i => s"value$i")
+
+  private val jKeys = List.tabulate(10)(i => JAttributeKey.stringKey(s"key$i"))
+  private val jAttributes = List.tabulate(10) { i =>
+    val size = i + 1
+    val pairs = jKeys.take(size).zip(values.take(size))
+
+    pairs
+      .foldLeft(JAttributes.builder()) { case (builder, (key, value)) =>
+        builder.put(key, value)
+      }
+      .build()
+  }
+
+  private val keys = List.tabulate(10)(i => AttributeKey.string(s"key$i"))
+  private val attributes = List.tabulate(10) { i =>
+    val size = i + 1
+    val pairs = keys.take(size).zip(values.take(size))
+
+    pairs
+      .foldLeft(Attributes.newBuilder) { case (builder, (key, value)) =>
+        builder.addOne(key, value)
+      }
+      .result()
+  }


it's been so long since I've done any benchmarking, but I think we want these defined in the benchmark instance rather than the companion, and I think there's also a setup method where you're supposed to construct them to prevent some compiler optimisations that wouldn't happen in regular code

I took the idea of the benchmark from https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/jmh/java/io/opentelemetry/api/common/AttributesBenchmark.java#L25-L40.
Here, they are defined as static variables.

sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala

iRevive requested review from armanbilge and NthPortal November 21, 2023 10:17

iRevive changed the title ~~Add Builder of Attributes. Documentation improvements.~~ Add Builder of Attributes. Documentation improvements Nov 21, 2023

iRevive force-pushed the attributes-refactor branch from 3af9641 to bbbf748 Compare November 21, 2023 11:21

iRevive force-pushed the attributes-refactor branch 4 times, most recently from 7f23bec to 6814e88 Compare November 21, 2023 11:40

Refactor Attributes

b821ea7

iRevive force-pushed the attributes-refactor branch from 6814e88 to b821ea7 Compare November 21, 2023 12:06

iRevive commented Nov 21, 2023

View reviewed changes

NthPortal reviewed Nov 21, 2023

View reviewed changes

sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala Show resolved Hide resolved

Make Attributes extend Iterable[Attribute[_]]

051fbc9

iRevive force-pushed the attributes-refactor branch from 0277171 to 051fbc9 Compare November 22, 2023 08:26

Add attributes benchmarks

dc7a2a8

Remove redundant methods from Attributes

797e983

NthPortal reviewed Nov 22, 2023

View reviewed changes

iRevive requested a review from NthPortal November 23, 2023 10:11

NthPortal approved these changes Nov 24, 2023

View reviewed changes

sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala Show resolved Hide resolved

sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala Show resolved Hide resolved

Add overloaded addOne to the attributes builder

6f0f4ab

iRevive merged commit c768309 into typelevel:main Nov 25, 2023
10 checks passed

iRevive deleted the attributes-refactor branch November 25, 2023 08:12

NthPortal mentioned this pull request Nov 28, 2023

Support using Scala propagators with Java backend #365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Builder` of `Attributes`. Documentation improvements #382

Add `Builder` of `Attributes`. Documentation improvements #382

iRevive commented Nov 21, 2023

NthPortal commented Nov 21, 2023

iRevive commented Nov 21, 2023

iRevive commented Nov 21, 2023

NthPortal commented Nov 21, 2023 •

edited

Loading

iRevive commented Nov 21, 2023

iRevive commented Nov 21, 2023

iRevive Nov 21, 2023

NthPortal Nov 21, 2023

iRevive Nov 22, 2023

NthPortal Nov 21, 2023

iRevive Nov 22, 2023

NthPortal Nov 22, 2023

NthPortal commented Nov 21, 2023 •

edited

Loading

iRevive commented Nov 22, 2023

NthPortal commented Nov 22, 2023 •

edited

Loading

iRevive commented Nov 22, 2023

NthPortal commented Nov 22, 2023

iRevive commented Nov 22, 2023

NthPortal Nov 22, 2023

iRevive Nov 22, 2023


		def iterator: Iterator[Attribute[_]] = m.values.iterator

		override def toList: List[Attribute[_]] = m.values.toList

Add Builder of Attributes. Documentation improvements #382

Add Builder of Attributes. Documentation improvements #382

Conversation

iRevive commented Nov 21, 2023

NthPortal commented Nov 21, 2023

iRevive commented Nov 21, 2023

iRevive commented Nov 21, 2023

NthPortal commented Nov 21, 2023 • edited Loading

iRevive commented Nov 21, 2023

iRevive commented Nov 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthPortal commented Nov 21, 2023 • edited Loading

iRevive commented Nov 22, 2023

NthPortal commented Nov 22, 2023 • edited Loading

iRevive commented Nov 22, 2023

NthPortal commented Nov 22, 2023

iRevive commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Builder` of `Attributes`. Documentation improvements #382

Add `Builder` of `Attributes`. Documentation improvements #382

NthPortal commented Nov 21, 2023 •

edited

Loading

NthPortal commented Nov 21, 2023 •

edited

Loading

NthPortal commented Nov 22, 2023 •

edited

Loading