Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Builder of Attributes. Documentation improvements #382

Merged
merged 5 commits into from
Nov 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/*
* Copyright 2022 Typelevel
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.typelevel.otel4s.benchmarks

import io.opentelemetry.api.common.{AttributeKey => JAttributeKey}
import io.opentelemetry.api.common.{Attributes => JAttributes}
import org.openjdk.jmh.annotations._
import org.typelevel.otel4s.Attribute
import org.typelevel.otel4s.AttributeKey
import org.typelevel.otel4s.sdk.Attributes

import java.util.concurrent.TimeUnit

// benchmarks/Jmh/run org.typelevel.otel4s.benchmarks.AttributesBenchmark -prof gc
@State(Scope.Thread)
@BenchmarkMode(Array(Mode.AverageTime))
@Fork(1)
@Measurement(iterations = 15, time = 1)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Warmup(iterations = 5, time = 1)
class AttributesBenchmark {
import AttributesBenchmark._

@Benchmark
def java_computeHashCode(): Unit = {
for (a <- jAttributes) { a.hashCode() }
}

@Benchmark
def java_ofOne(): JAttributes =
JAttributes.of(jKeys.head, values.head)

@Benchmark
def java_ofFive(): JAttributes =
JAttributes.of(
jKeys(0),
values(0),
jKeys(1),
values(1),
jKeys(2),
values(2),
jKeys(3),
values(3),
jKeys(4),
values(4)
)

@Benchmark
def java_builderTenItem(): JAttributes = {
val builder = JAttributes.builder()
for (i <- 0 until 10) {
builder.put(jKeys(i), values(i))
}
builder.build()
}

@Benchmark
def otel4s_computeHashCode(): Unit = {
for (a <- attributes) {
a.hashCode()
}
}

@Benchmark
def otel4s_ofOne(): Attributes =
Attributes(Attribute(keys.head, values.head))

@Benchmark
def otel4s_ofFive(): Attributes =
Attributes(
Attribute(keys(0), values(0)),
Attribute(keys(1), values(1)),
Attribute(keys(2), values(2)),
Attribute(keys(3), values(3)),
Attribute(keys(4), values(4))
)

@Benchmark
def otel4s_builderTenItem(): Attributes = {
val builder = Attributes.newBuilder
for (i <- 0 until 10) {
builder.addOne(keys(i), values(i))
}
builder.result()
}
}

object AttributesBenchmark {
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()
}
Comment on lines +103 to +127
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
2 changes: 1 addition & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ lazy val benchmarks = project
.enablePlugins(NoPublishPlugin)
.enablePlugins(JmhPlugin)
.in(file("benchmarks"))
.dependsOn(core.jvm, java, testkit.jvm)
.dependsOn(core.jvm, sdk.jvm, java, testkit.jvm)
.settings(
name := "otel4s-benchmarks"
)
Expand Down
222 changes: 177 additions & 45 deletions sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Attributes.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,49 +16,39 @@

package org.typelevel.otel4s.sdk

import cats.Applicative
import cats.Hash
import cats.Monad
import cats.Monoid
import cats.Show
import cats.implicits._
import cats.syntax.show._
import org.typelevel.otel4s.Attribute
import org.typelevel.otel4s.Attribute.KeySelect
import org.typelevel.otel4s.AttributeKey

/** An immutable collection of [[Attribute]]s.
import scala.collection.SpecificIterableFactory
import scala.collection.mutable

/** An immutable collection of [[Attribute]]s. It contains only unique keys.
*
* @see
* [[https://opentelemetry.io/docs/specs/otel/common/#attribute-collections]]
*/
final class Attributes private (
private val m: Map[AttributeKey[_], Attribute[_]]
) {
def get[T: KeySelect](name: String): Option[Attribute[T]] = {
val key = KeySelect[T].make(name)
m.get(key).map(_.asInstanceOf[Attribute[T]])
}
def get[T](key: AttributeKey[T]): Option[Attribute[T]] =
m.get(key).map(_.asInstanceOf[Attribute[T]])

def isEmpty: Boolean = m.isEmpty
def size: Int = m.size
def contains(key: AttributeKey[_]): Boolean = m.contains(key)
def foldLeft[F[_]: Monad, B](z: B)(f: (B, Attribute[_]) => F[B]): F[B] =
m.foldLeft(Monad[F].pure(z)) { (acc, v) =>
acc.flatMap { b =>
f(b, v._2)
}
}
def forall[F[_]: Monad](p: Attribute[_] => F[Boolean]): F[Boolean] =
foldLeft(true)((b, a) => {
if (b) p(a).map(b && _)
else Monad[F].pure(false)
})
def foreach[F[_]: Applicative](f: Attribute[_] => F[Unit]): F[Unit] =
m.foldLeft(Applicative[F].unit) { (acc, v) =>
acc *> f(v._2)
}
sealed trait Attributes extends Iterable[Attribute[_]] {
NthPortal marked this conversation as resolved.
Show resolved Hide resolved

def toMap: Map[AttributeKey[_], Attribute[_]] = m
def toList: List[Attribute[_]] = m.values.toList
/** Returns an attribute for the given attribute name, or `None` if not found.
*/
def get[T: KeySelect](name: String): Option[Attribute[T]]

/** Returns an attribute for the given attribute key, or `None` if not found.
*/
def get[T](key: AttributeKey[T]): Option[Attribute[T]]

/** Whether this attributes collection contains the given key.
*/
def contains(key: AttributeKey[_]): Boolean

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collections use to{X}

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


override def hashCode(): Int =
Hash[Attributes].hash(this)
Expand All @@ -73,17 +63,43 @@ final class Attributes private (
Show[Attributes].show(this)
}

object Attributes {
object Attributes extends SpecificIterableFactory[Attribute[_], Attributes] {
private val Empty = new MapAttributes(Map.empty)

/** Creates [[Attributes]] with the given `attributes`.
*
* @note
* if there are duplicated keys in the given `attributes`, only the last
* occurrence will be retained.
*
* @param attributes
* the attributes to use
*/
override def apply(attributes: Attribute[_]*): Attributes =
fromSpecific(attributes)
NthPortal marked this conversation as resolved.
Show resolved Hide resolved

val Empty = new Attributes(Map.empty)
/** Creates an empty [[Builder]] of [[Attributes]].
*/
def newBuilder: Builder = new Builder

def apply(attributes: Attribute[_]*): Attributes = {
val builder = Map.newBuilder[AttributeKey[_], Attribute[_]]
attributes.foreach { a =>
builder += (a.key -> a)
/** Returns empty [[Attributes]].
*/
def empty: Attributes = Empty

/** Creates [[Attributes]] from the given collection.
*
* @note
* if there are duplicated keys in the given `attributes`, only the last
* occurrence will be retained.
*
* @param attributes
* the attributes to use
*/
def fromSpecific(attributes: IterableOnce[Attribute[_]]): Attributes =
attributes match {
case a: Attributes => a
case other => (newBuilder ++= other).result()
}
new Attributes(builder.result())
}

implicit val showAttributes: Show[Attributes] = Show.show { attributes =>
attributes.toList
Expand All @@ -92,15 +108,131 @@ object Attributes {
}

implicit val hashAttributes: Hash[Attributes] =
Hash.by(_.m)
Hash.by(_.toMap)

implicit val monoidAttributes: Monoid[Attributes] =
new Monoid[Attributes] {
def empty: Attributes = Attributes.Empty
def combine(x: Attributes, y: Attributes): Attributes = {
def combine(x: Attributes, y: Attributes): Attributes =
if (y.isEmpty) x
else if (x.isEmpty) y
else new Attributes(x.m ++ y.m)
else new MapAttributes(x.toMap ++ y.toMap)
}

/** A '''mutable''' builder of [[Attributes]].
*/
final class Builder extends mutable.Builder[Attribute[_], Attributes] {
private val builder = Map.newBuilder[AttributeKey[_], Attribute[_]]

/** Adds the attribute with the given `key` and `value` to the builder.
*
* @note
* if the given `key` is already present in the builder, the value will
* be overwritten with the given `value`.
*
* @param key
* the key of the attribute. Denotes the types of the `value`
*
* @param value
* the value of the attribute
*/
def addOne[A](key: AttributeKey[A], value: A): this.type = {
NthPortal marked this conversation as resolved.
Show resolved Hide resolved
builder.addOne((key, Attribute(key, value)))
this
}

/** Adds the attribute with the given `key` (created from `name`) and
* `value` to the builder.
*
* @note
* if the given `key` is already present in the builder, the value will
* be overwritten with the given `value`.
*
* @param name
* the name of the attribute's key
*
* @param value
* the value of the attribute
*/
def addOne[A: KeySelect](name: String, value: A): this.type = {
val key = KeySelect[A].make(name)
builder.addOne((key, Attribute(key, value)))
this
}

/** Adds the given `attribute` to the builder.
*
* @note
* if the key of the given `attribute` is already present in the builder,
* the value will be overwritten with the corresponding given attribute.
*
* @param attribute
* the attribute to add
*/
def addOne(attribute: Attribute[_]): this.type = {
builder.addOne((attribute.key, attribute))
this
}

/** Adds the given `attributes` to the builder.
*
* @note
* if the keys of the given `attributes` are already present in the
* builder, the values will be overwritten with the corresponding given
* attributes.
*
* @param attributes
* the attributes to add
*/
override def addAll(attributes: IterableOnce[Attribute[_]]): this.type = {
attributes match {
case a: Attributes => builder.addAll(a.toMap)
case other => super.addAll(other)
}
this
}

override def sizeHint(size: Int): Unit =
builder.sizeHint(size)

def clear(): Unit =
builder.clear()

/** Creates [[Attributes]] with the attributes of this builder.
*/
def result(): Attributes =
new MapAttributes(builder.result())
}

private final class MapAttributes(
private val m: Map[AttributeKey[_], Attribute[_]]
) extends Attributes {
def get[T: KeySelect](name: String): Option[Attribute[T]] = {
val key = KeySelect[T].make(name)
m.get(key).map(_.asInstanceOf[Attribute[T]])
}

def get[T](key: AttributeKey[T]): Option[Attribute[T]] =
m.get(key).map(_.asInstanceOf[Attribute[T]])

def contains(key: AttributeKey[_]): Boolean = m.contains(key)
def toMap: Map[AttributeKey[_], Attribute[_]] = m
def iterator: Iterator[Attribute[_]] = m.valuesIterator

override def isEmpty: Boolean = m.isEmpty
override def size: Int = m.size
override def knownSize: Int = m.knownSize
override def empty: Attributes = Attributes.empty
override def toList: List[Attribute[_]] = m.values.toList
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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.


override protected def fromSpecific(
coll: IterableOnce[Attribute[_]]
): Attributes =
Attributes.fromSpecific(coll)

override protected def newSpecificBuilder
: mutable.Builder[Attribute[_], Attributes] =
Attributes.newBuilder
}

}
Loading