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

core and sdk: Add Hash and Show instances #331

Merged
merged 5 commits into from
Nov 1, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 3 additions & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ lazy val `core-trace` = crossProject(JVMPlatform, JSPlatform, NativePlatform)
name := "otel4s-core-trace",
libraryDependencies ++= Seq(
"org.typelevel" %%% "cats-effect-kernel" % CatsEffectVersion,
"org.scodec" %%% "scodec-bits" % ScodecVersion
"org.scodec" %%% "scodec-bits" % ScodecVersion,
"org.scalameta" %%% "munit-scalacheck" % MUnitVersion % Test
)
)
.settings(scalafixSettings)
Expand Down Expand Up @@ -169,6 +170,7 @@ lazy val `sdk-trace` = crossProject(JVMPlatform, JSPlatform, NativePlatform)
name := "otel4s-sdk-trace",
libraryDependencies ++= Seq(
"org.typelevel" %%% "cats-effect" % CatsEffectVersion,
"org.scalameta" %%% "munit-scalacheck" % MUnitVersion % Test
),
)
.settings(munitDependencies)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@

package org.typelevel.otel4s

import cats.Hash
import cats.Show
import cats.implicits.showInterpolator
import cats.kernel.Hash
import cats.syntax.show._

/** Represents the key-value attribute.
*
Expand Down Expand Up @@ -76,4 +76,8 @@ String, Boolean, Long, Double, List[String], List[Boolean], List[Long], List[Dou
implicit def hashAttribute[T: Hash]: Hash[Attribute[T]] =
Hash.by(a => (a.key, a.value))

implicit val hashAnyAttribute: Hash[Attribute[_]] = {
implicit val hashAny: Hash[Any] = Hash.fromUniversalHashCode
Hash.by(a => (a.key, a.value))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,7 @@ object AttributeKey {
implicit def attributeKeyShow[A]: Show[AttributeKey[A]] =
Show.show(key => show"${key.`type`}(${key.name})")

implicit val attributeKeyAnyHash: Hash[AttributeKey[_]] =
Hash.by(key => (key.name, key.`type`))
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit weird, we definitely don't want to keep it gated in a testkit or behind an extra import?

Either way, Any is distinct from _ which I think is called "existential".

Copy link
Contributor Author

@iRevive iRevive Oct 3, 2023

Choose a reason for hiding this comment

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

I guess we can keep it behind the certain import.

There are several places in (upcoming) Otel4s SDK where we don't know a concrete type of an Attritube

Copy link
Contributor Author

@iRevive iRevive Oct 3, 2023

Choose a reason for hiding this comment

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

Could you suggest the object's name where I can place this implicit, please?

Should the structure be similar to something like this?:

object AttributeKey {
  object Implicits {
    object Extra {
      implicit val attributeKeyExistentialHash: Hash[AttributeKey[_]] = ...
    }
  }
}

Extra / Existential / Unsafe / Internal?

Copy link
Contributor Author

@iRevive iRevive Oct 4, 2023

Choose a reason for hiding this comment

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

Hm, we already define some Show[Attribute[_]] instances in the public implicit lookup scope:

implicit val showAttribute: Show[Attribute[_]] = (a: Attribute[_]) =>
.

Copy link
Contributor Author

@iRevive iRevive Oct 11, 2023

Choose a reason for hiding this comment

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

@armanbilge what are the implications if we keep existential instances in the public scope?

We need existential instances for:

  1. AttributeType[_] - a sealed trait with case objects, universal hashcode is fine here
  2. AttributeKey[_] - created from key => (key.name, key.type), where the name is a String and the type is an AttributeType[_] (a coproduct)
  3. Attribute[_] - created from a => (a.key, a.value), where the key is AttributeKey, the value is one of Boolean, Double, String, Long, List[Boolean], List[Double], List[String], List[Long].

The Hash[Attribute[List[String]]] should generate the same hashcode as Hash[Attribute[_]].

Copy link
Member

Choose a reason for hiding this comment

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

If we are confident that universal hashing will work for all the possible types (now and future) then it seems fine. e.g. could there ever be an attribute for a CIString?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec says the following:

The attribute value is either:

  • A primitive type: string, boolean, double precision floating point (IEEE 754-1985) or signed 64 bit integer.
  • An array of primitive type values. The array MUST be homogeneous, i.e., it MUST NOT contain values of different types.

I don't think we should allow any other types other than these.
So, I doubt Attribute[CIString] should be a thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

regardless, CIString has a case-insensitive hashCode implementation, so universal hashing should still work anyway

Copy link
Member

Choose a reason for hiding this comment

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

regardless, CIString has a case-insensitive hashCode implementation

Yes, it does today, but I'm considering a future where we might want to make it an opaque type.


}
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,7 @@ object AttributeType {
implicit def attributeTypeShow[A]: Show[AttributeType[A]] =
Show.fromToString

implicit val attributeTypeAnyHash: Hash[AttributeType[_]] =
Hash.fromUniversalHashCode

}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
package org.typelevel.otel4s
package trace

import cats.Hash
import cats.Show

/** Type of [[Span]]. Can be used to specify additional relationships between
* spans in addition to a parent/child relationship.
*/
Expand Down Expand Up @@ -48,4 +51,7 @@ object SpanKind {
* relationship between producer and consumer spans.
*/
case object Consumer extends SpanKind

implicit val spanKindHash: Hash[SpanKind] = Hash.fromUniversalHashCode
implicit val spanKindShow: Show[SpanKind] = Show.fromToString
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package org.typelevel.otel4s.trace

import cats.Hash
import cats.Show

/** The set of canonical status codes
*/
sealed trait Status extends Product with Serializable
Expand All @@ -33,4 +36,7 @@ object Status {
/** The operation contains an error. */
case object Error extends Status

implicit val statusHash: Hash[Status] = Hash.fromUniversalHashCode
implicit val statusShow: Show[Status] = Show.fromToString

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* 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.trace

import cats.Hash
import cats.Show
import munit._
import org.scalacheck.Gen
import org.scalacheck.Prop

class SpanKindSuite extends ScalaCheckSuite {

private val spanKindGen: Gen[SpanKind] =
Gen.oneOf(
SpanKind.Internal,
SpanKind.Server,
SpanKind.Client,
SpanKind.Producer,
SpanKind.Consumer
)

property("Show[SpanKind]") {
Prop.forAll(spanKindGen) { spanKind =>
val expected = spanKind match {
case SpanKind.Internal => "Internal"
case SpanKind.Server => "Server"
case SpanKind.Client => "Client"
case SpanKind.Producer => "Producer"
case SpanKind.Consumer => "Consumer"
}

assertEquals(Show[SpanKind].show(spanKind), expected)
}
}

test("Hash[SpanKind]") {
iRevive marked this conversation as resolved.
Show resolved Hide resolved
val allWithIndex = List(
SpanKind.Internal,
SpanKind.Server,
SpanKind.Client,
SpanKind.Producer,
SpanKind.Consumer
).zipWithIndex

allWithIndex.foreach { case (that, thatIdx) =>
allWithIndex.foreach { case (other, otherIdx) =>
assertEquals(Hash[SpanKind].eqv(that, other), thatIdx == otherIdx)
}
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* 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.trace

import cats.Hash
import cats.Show
import munit._
import org.scalacheck.Gen
import org.scalacheck.Prop

class StatusSuite extends ScalaCheckSuite {

private val statusGen: Gen[Status] =
Gen.oneOf(Status.Unset, Status.Ok, Status.Error)

property("Show[Status]") {
Prop.forAll(statusGen) { status =>
val expected = status match {
case Status.Unset => "Unset"
case Status.Ok => "Ok"
case Status.Error => "Error"
}

assertEquals(Show[Status].show(status), expected)
}
}

test("Hash[Status]") {
val allWithIndex = List(Status.Unset, Status.Ok, Status.Error).zipWithIndex

allWithIndex.foreach { case (that, thatIdx) =>
allWithIndex.foreach { case (other, otherIdx) =>
assertEquals(Hash[Status].eqv(that, other), thatIdx == otherIdx)
}
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.typelevel.otel4s.sdk

import cats.Applicative
import cats.Hash
import cats.Monad
import cats.Monoid
import cats.Show
Expand Down Expand Up @@ -59,6 +60,17 @@ final class Attributes private (
def toMap: Map[AttributeKey[_], Attribute[_]] = m
def toList: List[Attribute[_]] = m.values.toList

override def hashCode(): Int =
Hash[Attributes].hash(this)

override def equals(obj: Any): Boolean =
obj match {
case other: Attributes => Hash[Attributes].eqv(this, other)
case _ => false
}

override def toString: String =
Show[Attributes].show(this)
}

object Attributes {
Expand All @@ -79,6 +91,9 @@ object Attributes {
.mkString("Attributes(", ", ", ")")
}

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

implicit val monoidAttributes: Monoid[Attributes] =
new Monoid[Attributes] {
def empty: Attributes = Attributes.Empty
Expand Down
17 changes: 12 additions & 5 deletions sdk/common/src/main/scala/org/typelevel/otel4s/sdk/Resource.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@

package org.typelevel.otel4s.sdk

import cats.Hash
import cats.Show
import cats.implicits.catsSyntaxEitherId
import cats.implicits.catsSyntaxOptionId
import cats.implicits.catsSyntaxSemigroup
import cats.implicits.showInterpolator
import cats.syntax.either._
import cats.syntax.option._
import cats.syntax.semigroup._
import cats.syntax.show._
NthPortal marked this conversation as resolved.
Show resolved Hide resolved
import org.typelevel.otel4s.Attribute
import org.typelevel.otel4s.sdk.Resource.ResourceInitiationError
import org.typelevel.otel4s.semconv.resource.attributes.ResourceAttributes._
Expand Down Expand Up @@ -76,6 +77,9 @@ final case class Resource(
throw _,
identity
)

override def toString: String =
Show[Resource].show(this)
}

object Resource {
Expand Down Expand Up @@ -119,6 +123,9 @@ object Resource {
val Default: Resource = TelemetrySdk.mergeIntoUnsafe(Mandatory)

implicit val showResource: Show[Resource] =
r => show"Resource(${r.attributes}, ${r.schemaUrl})"
r => show"Resource{attributes=${r.attributes}, schemaUrl=${r.schemaUrl}}"

implicit val hashResource: Hash[Resource] =
Hash.by(r => (r.attributes, r.schemaUrl))

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@

package org.typelevel.otel4s.sdk.trace.samplers

import cats.Hash
import cats.Show

/** A decision on whether a span should be recorded, sampled, or dropped.
*/
sealed abstract class SamplingDecision(val isSampled: Boolean)
Expand All @@ -35,4 +38,11 @@ object SamplingDecision {
/** The span is recorded, and the Sampled flag will be set.
*/
case object RecordAndSample extends SamplingDecision(true)

implicit val samplingDecisionHash: Hash[SamplingDecision] =
Hash.fromUniversalHashCode

implicit val samplingDecisionShow: Show[SamplingDecision] =
Show.fromToString

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,60 @@

package org.typelevel.otel4s.sdk.trace.samplers

import cats.Hash
import cats.Show
import munit._
import org.scalacheck.Gen
import org.scalacheck.Prop

class SamplingDecisionSuite extends FunSuite {
test("Drop should have isSampled = false") {
assertEquals(SamplingDecision.Drop.isSampled, false)
class SamplingDecisionSuite extends ScalaCheckSuite {

private val samplingDecisionGen: Gen[SamplingDecision] =
Gen.oneOf(
SamplingDecision.Drop,
SamplingDecision.RecordOnly,
SamplingDecision.RecordAndSample
)

property("is sampled") {
Prop.forAll(samplingDecisionGen) { decision =>
val expected = decision match {
case SamplingDecision.Drop => false
case SamplingDecision.RecordOnly => false
case SamplingDecision.RecordAndSample => true
}

assertEquals(decision.isSampled, expected)
}
}

test("RecordOnly should have isSampled = false") {
assertEquals(SamplingDecision.RecordOnly.isSampled, false)
property("Show[SamplingDecision]") {
Prop.forAll(samplingDecisionGen) { decision =>
val expected = decision match {
case SamplingDecision.Drop => "Drop"
case SamplingDecision.RecordOnly => "RecordOnly"
case SamplingDecision.RecordAndSample => "RecordAndSample"
}

assertEquals(Show[SamplingDecision].show(decision), expected)
}
}

test("RecordAndSample should have isSampled = true") {
assertEquals(SamplingDecision.RecordAndSample.isSampled, true)
test("Hash[SamplingDecision]") {
val allWithIndex = List(
SamplingDecision.Drop,
SamplingDecision.RecordOnly,
SamplingDecision.RecordAndSample
).zipWithIndex

allWithIndex.foreach { case (that, thatIdx) =>
allWithIndex.foreach { case (other, otherIdx) =>
assertEquals(
Hash[SamplingDecision].eqv(that, other),
thatIdx == otherIdx
)
}
}
}

}