Skip to content

Commit

Permalink
Partially redesign Attributes
Browse files Browse the repository at this point in the history 
Disallow having multiple `Attribute`s with the same name, even if
they are of different types.

Simplify `Attributes` by no longer having its API treat it like a
map—rename `updated` to `added`, make `toMap` package private, and
remove the following methods:
`contains`, `removed`, `-`, `removedAll`, `--`, `keys`
  • Loading branch information
@NthPortal
NthPortal committed Feb 29, 2024
1 parent 98887a4 commit 5bd717a
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 115 deletions.
96 changes: 33 additions & 63 deletions core/common/src/main/scala/org/typelevel/otel4s/Attributes.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,58 +36,42 @@ sealed trait Attributes
extends immutable.Iterable[Attribute[_]]
with IterableOps[Attribute[_], immutable.Iterable, Attributes] {

/** Returns an attribute for the given attribute name, or `None` if not found.
/** @return
* an [[`Attribute`]] matching the given attribute name and type, or `None`
* if not found.
*/
final def get[T: KeySelect](name: String): Option[Attribute[T]] =
get(KeySelect[T].make(name))

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

/** Whether or not these `Attributes` contain a key with the given name and
* type.
*/
final def contains[T: KeySelect](name: String): Boolean =
contains(KeySelect[T].make(name))

/** Whether or not these `Attributes` contain the given key. */
def contains(key: AttributeKey[_]): Boolean

/** Adds an [[`Attribute`]] with the given name and value to these
* `Attributes`, replacing any `Attribute` with the same name and type if one
* exists.
*/
final def updated[T: KeySelect](name: String, value: T): Attributes =
updated(Attribute(name, value))
final def added[T: KeySelect](name: String, value: T): Attributes =
added(Attribute(name, value))

/** Adds an [[`Attribute`]] with the given key and value to these
* `Attributes`, replacing any `Attribute` with the same key if one exists.
*/
final def updated[T](key: AttributeKey[T], value: T): Attributes =
updated(Attribute(key, value))
final def added[T](key: AttributeKey[T], value: T): Attributes =
added(Attribute(key, value))

/** Adds the given [[`Attribute`]] to these `Attributes`, replacing any
* `Attribute` with the same key if one exists.
*/
def updated(attribute: Attribute[_]): Attributes
def added(attribute: Attribute[_]): Attributes

/** Adds the given [[`Attribute`]] to these `Attributes`, replacing any
* `Attribute` with the same key if one exists.
*/
final def +(attribute: Attribute[_]): Attributes =
updated(attribute)

/** Removes the [[`Attribute`]] with the given name and type, if present. */
final def removed[T: KeySelect](name: String): Attributes =
removed(KeySelect[T].make(name))

/** Removes the [[`Attribute`]] with the given key, if present. */
def removed(key: AttributeKey[_]): Attributes

/** Removes the [[`Attribute`]] with the given key, if present. */
final def -(key: AttributeKey[_]): Attributes =
removed(key)
added(attribute)

/** Invariant overload of
* [[scala.collection.IterableOps.concat `IterableOps#concat`]] that returns
Expand All @@ -98,7 +82,7 @@ sealed trait Attributes
* the resulting `Attributes`.
*/
def concat(that: IterableOnce[Attribute[_]]): Attributes =
attributesFactory.fromSpecific(this.view ++ that)
fromSpecific(this.view ++ that)

/** Invariant overload of [[scala.collection.IterableOps.++ `IterableOps#++`]]
* that returns `Attributes` rather than `Iterable`.
Expand All @@ -110,23 +94,12 @@ sealed trait Attributes
final def ++(that: IterableOnce[Attribute[_]]): Attributes =
concat(that)

/** Removes all attributes with any of the given keys. */
def removedAll(that: IterableOnce[AttributeKey[_]]): Attributes =
attributesFactory.fromSpecific((toMap -- that).values)

/** Removes all attributes with any of the given keys. */
final def --(that: IterableOnce[AttributeKey[_]]): Attributes =
removedAll(that)

/** @return the `Map` representation of these `Attributes` */
def toMap: Map[AttributeKey[_], Attribute[_]]

/** Equivalent to `toMap.keySet`.
/** For internal use only, for comparison and testing. May not be fast.
*
* @return
* the keys of the [[`Attribute`]]s
* the `Map` representation of these `Attributes`
*/
final def keys: Set[AttributeKey[_]] = toMap.keySet
private[otel4s] def toMap: Map[String, Attribute[_]]

/** A factory for creating `Attributes`. */
def attributesFactory: SpecificIterableFactory[Attribute[_], Attributes]
Expand Down Expand Up @@ -207,13 +180,13 @@ object Attributes extends SpecificIterableFactory[Attribute[_], Attributes] {
def combine(x: Attributes, y: Attributes): Attributes =
if (y.isEmpty) x
else if (x.isEmpty) y
else new MapAttributes(x.toMap ++ y.toMap)
else x ++ y
}

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

/** Adds the attribute with the given `key` and `value` to the builder.
*
Expand All @@ -228,7 +201,7 @@ object Attributes extends SpecificIterableFactory[Attribute[_], Attributes] {
* the value of the attribute
*/
def addOne[A](key: AttributeKey[A], value: A): this.type = {
builder.addOne((key, Attribute(key, value)))
builder.addOne(key.name -> Attribute(key, value))
this
}

Expand All @@ -247,7 +220,7 @@ object Attributes extends SpecificIterableFactory[Attribute[_], Attributes] {
*/
def addOne[A: KeySelect](name: String, value: A): this.type = {
val key = KeySelect[A].make(name)
builder.addOne((key, Attribute(key, value)))
builder.addOne(key.name -> Attribute(key, value))
this
}

Expand All @@ -261,7 +234,7 @@ object Attributes extends SpecificIterableFactory[Attribute[_], Attributes] {
* the attribute to add
*/
def addOne(attribute: Attribute[_]): this.type = {
builder.addOne((attribute.key, attribute))
builder.addOne(attribute.key.name -> attribute)
this
}

Expand All @@ -277,8 +250,8 @@ object Attributes extends SpecificIterableFactory[Attribute[_], Attributes] {
*/
override def addAll(attributes: IterableOnce[Attribute[_]]): this.type = {
attributes match {
case a: Attributes => builder.addAll(a.toMap)
case other => super.addAll(other)
case a: MapAttributes => builder.addAll(a.m)
case other => super.addAll(other)
}
this
}
Expand All @@ -296,26 +269,23 @@ object Attributes extends SpecificIterableFactory[Attribute[_], Attributes] {
}

private final class MapAttributes(
private val m: Map[AttributeKey[_], Attribute[_]]
private[Attributes] val m: Map[String, Attribute[_]]
) extends Attributes {
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 updated(attribute: Attribute[_]): Attributes =
new MapAttributes(m.updated(attribute.key, attribute))
def removed(key: AttributeKey[_]): Attributes =
new MapAttributes(m.removed(key))
m.get(key.name)
.filter(_.key.`type` == key.`type`)
.asInstanceOf[Option[Attribute[T]]]
def added(attribute: Attribute[_]): Attributes =
new MapAttributes(m.updated(attribute.key.name, attribute))
override def concat(that: IterableOnce[Attribute[_]]): Attributes =
that match {
case other: Attributes =>
new MapAttributes(m ++ other.toMap)
case other: MapAttributes =>
new MapAttributes(m ++ other.m)
case other =>
new MapAttributes(m ++ other.iterator.map(a => a.key -> a))
new MapAttributes(m ++ other.iterator.map(a => a.key.name -> a))
}
override def removedAll(that: IterableOnce[AttributeKey[_]]): Attributes =
new MapAttributes(m -- that)

def toMap: Map[AttributeKey[_], Attribute[_]] = m
private[otel4s] def toMap: Map[String, Attribute[_]] = m
def iterator: Iterator[Attribute[_]] = m.valuesIterator

def attributesFactory: Attributes.type = Attributes
Expand Down
70 changes: 19 additions & 51 deletions core/common/src/test/scala/org/typelevel/otel4s/AttributesProps.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class AttributesProps extends ScalaCheckSuite {

property("Attributes#size is equal to the number of unique keys") {
forAll(listOfAttributes) { attributes =>
val keysSet = attributes.map(_.key).toSet
val keysSet = attributes.map(_.key.name).toSet
val attrs = attributes.to(Attributes)

keysSet.size == attrs.size
Expand All @@ -37,22 +37,13 @@ class AttributesProps extends ScalaCheckSuite {

property("Attributes#isEmpty is true when there are no attributes") {
forAll(listOfAttributes) { attributes =>
val keysSet = attributes.map(_.key).toSet
val keysSet = attributes.map(_.key.name).toSet
val attrs = attributes.to(Attributes)

keysSet.isEmpty == attrs.isEmpty
}
}

property("Attributes#contains is true when the key is present") {
forAll(listOfAttributes) { attributes =>
val keysSet = attributes.map(_.key).toSet
val attrs = attributes.to(Attributes)

keysSet.forall(attrs.contains)
}
}

property("Attributes#foreach iterates over all attributes") {
forAll(listOfAttributes) { attributes =>
val attrs = attributes.to(Attributes)
Expand All @@ -69,7 +60,7 @@ class AttributesProps extends ScalaCheckSuite {
val attrs = attributes.to(Attributes)
val list = attrs.toList

list.size == attrs.size && list.forall(a => attrs.contains(a.key))
list.size == attrs.size && list.forall(a => attrs.get(a.key).isDefined)
}
}

Expand All @@ -94,82 +85,59 @@ class AttributesProps extends ScalaCheckSuite {
}
}

property("Attributes#toMap returns a map of all attributes") {
property("Attributes#toMap (internal) returns a map of all attributes") {
forAll(listOfAttributes) { attributes =>
val attrs = attributes.to(Attributes)
val map = attrs.toMap

map.size == attrs.size && map.forall { case (k, v) =>
attrs.contains(k) && attrs.get(k).contains(v)
map.size == attrs.size && map.values.forall { a =>
attrs.get(a.key).isDefined && attrs.get(a.key).contains(a)
}
}
}

property("Attributes#keys returns all of the attributes' keys") {
forAll(Gens.attributes) { attributes =>
attributes.keys == attributes.map(_.key).toSet
}
}

property(
"Attributes#updated (+) adds attributes and replaces existing ones"
"Attributes#added (+) adds attributes and replaces existing ones"
) {
forAll { (value1: String, value2: String) =>
val a1 = Attribute("key", value1)
val a2 = Attribute("key", value2)
val Key = AttributeKey.string("key")

val a1 = Key(value1)
val a2 = Key(value2)
val attrs1 = Attributes.empty + a1
val attrs2 = Attributes.empty + a2
val attrs12 = Attributes.empty + a1 + a2

val attrs1Contains = attrs1.get[String]("key").exists(_.value == value1)
val attrs2Contains = attrs2.get[String]("key").exists(_.value == value2)
val attrs1Contains = attrs1.get(Key).exists(_.value == value1)
val attrs2Contains = attrs2.get(Key).exists(_.value == value2)
val attrs12Checks =
attrs12.get[String]("key").exists(_.value == value2) &&
attrs12.get(Key).exists(_.value == value2) &&
attrs12.sizeIs == 1 &&
(value1 == value2 ||
attrs12
.get[String]("key")
.get(Key)
.forall(_.value != value1))

attrs1Contains && attrs2Contains && attrs12Checks
}
}

property("Attributes#removed (-) removes attributes") {
forAll { (value: String) =>
val a = Attribute("key", value)
val attrs = Attributes(a)

(attrs - a.key).isEmpty &&
attrs
.removed[Long]("key")
.get[String]("key")
.contains(a)
}
}

property("Attributes#concat (++) combines two sets of attributes") {
forAll(Gens.attributes, Gens.attributes) { (attributes1, attributes2) =>
val unique = attributes1.keys ++ attributes2.keys
val diff = attributes1.keys.intersect(attributes2.keys)
val unique = attributes1.toMap.keySet ++ attributes2.toMap.keySet
val overlap = attributes1.toMap.keySet.intersect(attributes2.toMap.keySet)

val combined = attributes1 ++ attributes2
val sizeIsEqual = combined.size == unique.size

val secondCollectionOverrodeValues = diff.forall { key =>
combined.get(key).contains(attributes2.get(key).get)
val secondCollectionOverrodeValues = overlap.forall { key =>
combined.toMap.get(key).exists(attributes2.toMap.get(key).contains)
}

sizeIsEqual && secondCollectionOverrodeValues
}
}

property("Attributes#removedAll (--) removes a set of attributes") {
forAll(Gens.attributes) { attributes =>
(attributes -- attributes.keys).isEmpty
}
}

property("Show[Attributes]") {
forAll(Gens.attributes) { attributes =>
val expected = attributes.toList
Expand Down
2 changes: 1 addition & 1 deletion sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ class SdkSpanBackendSuite extends CatsEffectSuite with ScalaCheckEffectSuite {
test(".getAttribute(:AttributeKey)") {
PropF.forAllF { (init: Attributes, extraAttrs: Attributes) =>
// 'init' and 'extra' may have attributes under the same key. we need only unique keys in extra
val extra = extraAttrs.filterNot(a => init.contains(a.key))
val extra = extraAttrs.filterNot(a => init.get(a.key).isDefined)

for {
span <- start(attributes = init)
Expand Down

0 comments on commit 5bd717a

Please sign in to comment.