From 5bd717ac81ea6260edc42e37c2040918bfec84be Mon Sep 17 00:00:00 2001 From: Marissa Date: Thu, 29 Feb 2024 18:56:34 -0500 Subject: [PATCH 1/2] Partially redesign `Attributes` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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` --- .../org/typelevel/otel4s/Attributes.scala | 96 +++++++------------ .../typelevel/otel4s/AttributesProps.scala | 70 ++++---------- .../sdk/trace/SdkSpanBackendSuite.scala | 2 +- 3 files changed, 53 insertions(+), 115 deletions(-) diff --git a/core/common/src/main/scala/org/typelevel/otel4s/Attributes.scala b/core/common/src/main/scala/org/typelevel/otel4s/Attributes.scala index 89649c660..3c69dbae1 100644 --- a/core/common/src/main/scala/org/typelevel/otel4s/Attributes.scala +++ b/core/common/src/main/scala/org/typelevel/otel4s/Attributes.scala @@ -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 @@ -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`. @@ -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] @@ -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. * @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 diff --git a/core/common/src/test/scala/org/typelevel/otel4s/AttributesProps.scala b/core/common/src/test/scala/org/typelevel/otel4s/AttributesProps.scala index 5bb346c21..66e3e15ca 100644 --- a/core/common/src/test/scala/org/typelevel/otel4s/AttributesProps.scala +++ b/core/common/src/test/scala/org/typelevel/otel4s/AttributesProps.scala @@ -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 @@ -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) @@ -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) } } @@ -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 diff --git a/sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala b/sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala index 4f3fa75bc..f4e40973b 100644 --- a/sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala +++ b/sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala @@ -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) From 995cbf1d17766df19e54c237fe393d56cc80318c Mon Sep 17 00:00:00 2001 From: Marissa Date: Fri, 1 Mar 2024 16:09:37 -0500 Subject: [PATCH 2/2] fixup! Partially redesign `Attributes` --- .../org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala b/sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala index f4e40973b..faf225459 100644 --- a/sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala +++ b/sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/SdkSpanBackendSuite.scala @@ -213,7 +213,10 @@ 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.get(a.key).isDefined) + val extra = { + val initMap = init.toMap + extraAttrs.filterNot(a => initMap.contains(a.key.name)) + } for { span <- start(attributes = init)