Merge pull request #801 from iRevive/sdk-common/component-registry-un…

…iquness

sdk/common/shared/src/main/scala/org/typelevel/otel4s/sdk/internal/ComponentRegistry.scala

@@ -24,7 +24,7 @@ import cats.effect.std.AtomicCell
 import cats.syntax.functor._
 import org.typelevel.otel4s.sdk.common.InstrumentationScope
 
-/** A registry that caches components by `key`, `version`, and `schemaUrl`.
+/** A registry that caches components by `key`, `version`, `schemaUrl`, and `attributes`.
   *
   * @tparam F
   *   the higher-kinded type of a polymorphic effect
@@ -34,12 +34,7 @@ import org.typelevel.otel4s.sdk.common.InstrumentationScope
   */
 private[sdk] sealed trait ComponentRegistry[F[_], A] {
 
-  /** Returns the component associated with the `name`, `version`, and `schemaUrl`.
-    *
-    * '''Note''': `attributes` are not part of component identity.
-    *
-    * Behavior is undefined when different `attributes` are provided where `name`, `version`, and `schemaUrl` are
-    * identical.
+  /** Returns the component associated with the `name`, `version`, `schemaUrl`, and `attributes`.
     *
     * @param name
     *   the name to associate with a component
@@ -53,12 +48,7 @@ private[sdk] sealed trait ComponentRegistry[F[_], A] {
     * @param attributes
     *   the attributes to associate with a component
     */
-  def get(
-      name: String,
-      version: Option[String],
-      schemaUrl: Option[String],
-      attributes: Attributes
-  ): F[A]
+  def get(name: String, version: Option[String], schemaUrl: Option[String], attributes: Attributes): F[A]
 
   /** Returns the collection of the registered components.
     */
@@ -80,44 +70,28 @@ private[sdk] object ComponentRegistry {
     * @tparam A
     *   the type of the component
     */
-  def create[F[_]: Concurrent, A](
-      buildComponent: InstrumentationScope => F[A]
-  ): F[ComponentRegistry[F, A]] =
+  def create[F[_]: Concurrent, A](buildComponent: InstrumentationScope => F[A]): F[ComponentRegistry[F, A]] =
     for {
-      cache <- AtomicCell[F].of(Map.empty[Key, A])
+      cache <- AtomicCell[F].of(Map.empty[InstrumentationScope, A])
     } yield new Impl(cache, buildComponent)
 
-  private final case class Key(
-      name: String,
-      version: Option[String],
-      schemaUrl: Option[String]
-  )
-
   private final class Impl[F[_]: Applicative, A](
-      cache: AtomicCell[F, Map[Key, A]],
+      cache: AtomicCell[F, Map[InstrumentationScope, A]],
       buildComponent: InstrumentationScope => F[A]
   ) extends ComponentRegistry[F, A] {
 
-    def get(
-        name: String,
-        version: Option[String],
-        schemaUrl: Option[String],
-        attributes: Attributes
-    ): F[A] =
+    def get(name: String, version: Option[String], schemaUrl: Option[String], attributes: Attributes): F[A] =
       cache.evalModify { cache =>
-        val key = Key(name, version, schemaUrl)
+        val scope = InstrumentationScope(name, version, schemaUrl, attributes)
 
-        cache.get(key) match {
+        cache.get(scope) match {
           case Some(component) =>
             Applicative[F].pure((cache, component))
 
           case None =>
-            val scope =
-              InstrumentationScope(name, version, schemaUrl, attributes)
-
             for {
               component <- buildComponent(scope)
-            } yield (cache.updated(key, component), component)
+            } yield (cache.updated(scope, component), component)
         }
       }
 

sdk/common/shared/src/test/scala/org/typelevel/otel4s/sdk/internal/ComponentRegistrySuite.scala

@@ -30,12 +30,14 @@ class ComponentRegistrySuite extends CatsEffectSuite {
 
   registryTest("get cached values (by name only)") { registry =>
     for {
+      v0 <- registry.get(name, None, None, Attributes.empty)
       v1 <- registry.get(name, None, None, Attributes.empty)
       v2 <- registry.get(name, None, None, attributes)
       v3 <- registry.get(name, Some(version), None, attributes)
       v4 <- registry.get(name, Some(version), Some(schemaUrl), attributes)
     } yield {
-      assertEquals(v1, v2)
+      assertEquals(v0, v1)
+      assertNotEquals(v1, v2)
       assertNotEquals(v1, v3)
       assertNotEquals(v2, v3)
       assertNotEquals(v1, v4)
@@ -45,26 +47,37 @@ class ComponentRegistrySuite extends CatsEffectSuite {
 
   registryTest("get cached values (by name and version)") { registry =>
     for {
+      v0 <- registry.get(name, Some(version), None, Attributes.empty)
       v1 <- registry.get(name, Some(version), None, Attributes.empty)
       v2 <- registry.get(name, Some(version), None, attributes)
       v3 <- registry.get(name, Some(version), Some(schemaUrl), attributes)
     } yield {
-      assertEquals(v1, v2)
+      assertEquals(v0, v1)
+      assertNotEquals(v1, v2)
       assertNotEquals(v1, v3)
       assertNotEquals(v2, v3)
     }
   }
 
   registryTest("get cached values (by name, version, and schema)") { registry =>
     for {
+      v0 <- registry.get(name, Some(version), Some(schemaUrl), Attributes.empty)
       v1 <- registry.get(name, Some(version), Some(schemaUrl), Attributes.empty)
       v2 <- registry.get(name, Some(version), Some(schemaUrl), attributes)
+    } yield {
+      assertEquals(v0, v1)
+      assertNotEquals(v1, v2)
+    }
+  }
+
+  registryTest("get cached values (by name, version, schema, and attributes)") { registry =>
+    for {
+      v1 <- registry.get(name, Some(version), Some(schemaUrl), attributes)
+      v2 <- registry.get(name, Some(version), Some(schemaUrl), attributes)
     } yield assertEquals(v1, v2)
   }
 
-  private def registryTest(
-      name: String
-  )(body: ComponentRegistry[IO, TestComponent] => IO[Unit]): Unit =
+  private def registryTest(name: String)(body: ComponentRegistry[IO, TestComponent] => IO[Unit]): Unit =
     test(name) {
       for {
         registry <- ComponentRegistry.create(_ => IO.pure(new TestComponent()))