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

sdk-common: add ComponentRegistry #375

Closed
Closed
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
2 changes: 1 addition & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ lazy val `sdk-common` = crossProject(JVMPlatform, JSPlatform, NativePlatform)
name := "otel4s-sdk-common",
startYear := Some(2023),
libraryDependencies ++= Seq(
"org.typelevel" %%% "cats-effect-kernel" % CatsEffectVersion,
"org.typelevel" %%% "cats-effect-std" % CatsEffectVersion,
"org.typelevel" %%% "cats-mtl" % CatsMtlVersion,
"org.typelevel" %%% "cats-laws" % CatsVersion % Test,
"org.typelevel" %%% "discipline-munit" % DisciplineMUnitVersion % Test,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
/*
* Copyright 2023 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.sdk
package internal

import cats.Applicative
import cats.effect.kernel.Concurrent
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`.
*
* @tparam F
* the higher-kinded type of a polymorphic effect
*
* @tparam A
* the type of the component
*/
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.
*
* @param name
* the name to associate with a component
*
* @param version
* the version to associate with a component
*
* @param schemaUrl
* the schema URL to associate with a component
*
* @param attributes
* the attributes to associate with a component
*/
def get(
name: String,
version: Option[String],
schemaUrl: Option[String],
attributes: Attributes
): F[A]

}

object ComponentRegistry {

/** Creates a [[ComponentRegistry]] that uses `buildComponent` to build a
* component if it is not already present in the cache.
*
* @param buildComponent
* how to build a component
*
* @tparam F
* the higher-kinded type of a polymorphic effect
*
* @tparam A
* the type of the component
*/
def create[F[_]: Concurrent, A](
buildComponent: InstrumentationScope => F[A]
): F[ComponentRegistry[F, A]] =
for {
cache <- AtomicCell[F].of(Map.empty[Key, 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]],
buildComponent: InstrumentationScope => F[A]
) extends ComponentRegistry[F, A] {

def get(
name: String,
version: Option[String],
schemaUrl: Option[String],
attributes: Attributes
): F[A] =
cache.evalModify { cache =>
Copy link
Member

@armanbilge armanbilge Nov 16, 2023

Choose a reason for hiding this comment

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

This implementation is a bit more defensive than the Java implementation from what I can make out.

Specifically, you are guaranteeing that buildComponent(...) will never be run twice for the same scope. However, the Java implementation doesn't actually seem to be guaranteeing that.

https://github.com/open-telemetry/opentelemetry-java/blob/9ac678e81bce7c12820acdb846d22d7957b8b15f/sdk/common/src/main/java/io/opentelemetry/sdk/internal/ComponentRegistry.java#L115-L121

Copy link
Contributor

@NthPortal NthPortal Nov 17, 2023

Choose a reason for hiding this comment

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

I believe you're mistaken. it took a bit of digging, but it seems that ConcurrentHashMap#computeIfAbsent is guaranteed to execute the function exactly once if the key is absent, and exactly zero times if already present. this is specific to ConcurrentHashMap and not true of all ConcurrentMap implementations (e.g. ConcurrentSkipListMap)

Copy link
Member

Choose a reason for hiding this comment

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

Aha you're right, thanks for taking a close look. I missed that the component is being added to two collections, and the lock is specifically for the allComponents collection.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps this is nitpicky, but I feel like it would be clearer without the name shadowing. perhaps map instead of cache for the function parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

after thinking about it, it might be worth having a call to cache.get outside of the evalModify so that we can opportunistically avoid blocking if the cache is already populated

val key = Key(name, version, schemaUrl)

cache.get(key) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

some part of me is going "but could this be implemented using updatedWith?" but idk if it makes the code any cleaner or is worth the effort

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)
}
}

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Copyright 2023 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.sdk.internal

import cats.effect.IO
import munit.CatsEffectSuite
import org.typelevel.otel4s.Attribute
import org.typelevel.otel4s.sdk.Attributes

class ComponentRegistrySuite extends CatsEffectSuite {

private val name = "component"
private val version = "0.0.1"
private val schemaUrl = "https://otel4s.schema.com"
private val attributes = Attributes(Attribute("key", "value"))

registryTest("get cached values (by name only)") { registry =>
for {
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing a case in each of the three tests for name and schema only, as well as a fourth test for name and schema only

} yield {
assertEquals(v1, v2)
assertNotEquals(v1, v3)
assertNotEquals(v2, v3)
assertNotEquals(v1, v4)
assertNotEquals(v2, v4)
}
}

registryTest("get cached values (by name and version)") { registry =>
for {
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)
assertNotEquals(v1, v3)
assertNotEquals(v2, v3)
}
}

registryTest("get cached values (by name, version, and schema)") { registry =>
for {
v1 <- registry.get(name, Some(version), Some(schemaUrl), Attributes.Empty)
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 =
test(name) {
for {
registry <- ComponentRegistry.create(_ => IO.pure(new TestComponent()))
_ <- body(registry)
} yield ()
}

private class TestComponent

}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import munit.CatsEffectSuite
import org.typelevel.otel4s.trace.SpanContext

class IdGeneratorSuite extends CatsEffectSuite {
private val Attempts = 1_000_000
private val Attempts = 100_000

generatorTest("generate a valid trace id") { generator =>
generator.generateTraceId
Expand Down