-
Notifications
You must be signed in to change notification settings - Fork 38
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
Conversation
Hash
and Show
instancesHash
and Show
instances
core/trace/src/test/scala/org/typelevel/otel4s/trace/SpanKindSuite.scala
Outdated
Show resolved
Hide resolved
implicit val attributeKeyAnyHash: Hash[AttributeKey[_]] = | ||
Hash.by(key => (key.name, key.`type`)) |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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[_]) => |
There was a problem hiding this comment.
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:
AttributeType[_]
- a sealed trait with case objects, universal hashcode is fine hereAttributeKey[_]
- created fromkey => (key.name, key.type)
, where thename
is aString
and thetype
is anAttributeType[_]
(a coproduct)Attribute[_]
- created froma => (a.key, a.value)
, where thekey
isAttributeKey
, thevalue
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[_]]
.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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-insensitivehashCode
implementation
Yes, it does today, but I'm considering a future where we might want to make it an opaque type
.
@@ -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 hashAttributeExistential: Hash[Attribute[_]] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the attribute value can exactly be one of: Boolean
, Double
, String
, Long
, List[Boolean]
, List[Double]
, List[String]
, List[Long]
.
So using a universal hashcode should be safe here.
implicit val attributeExistentialCogen: Cogen[Attribute[_]] = | ||
Cogen { (seed, attr) => | ||
def primitive[A: Cogen](seed: Seed): Seed = | ||
Cogen[A].perturb(seed, attr.value.asInstanceOf[A]) | ||
|
||
def list[A: Cogen](seed: Seed): Seed = | ||
Cogen[List[A]].perturb(seed, attr.value.asInstanceOf[List[A]]) | ||
|
||
val valueCogen: Seed => Seed = attr.key.`type` match { | ||
case AttributeType.Boolean => primitive[Boolean] | ||
case AttributeType.Double => primitive[Double] | ||
case AttributeType.String => primitive[String] | ||
case AttributeType.Long => primitive[Long] | ||
case AttributeType.BooleanList => list[Boolean] | ||
case AttributeType.DoubleList => list[Double] | ||
case AttributeType.StringList => list[String] | ||
case AttributeType.LongList => list[Long] | ||
} | ||
|
||
valueCogen(attributeKeyCogen.perturb(seed, attr.key)) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@armanbilge I didn't use Cogen
before. Am I defining it correctly?
Do I understand this in the right way:
Gen
- produces a value of typeA
fromSeed
Cogen
- produces aSeed
from the given valueA
Is Cogen
used to adjust the seed for the structured values, and, as a result, produce better pseudo-random values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
Cogen
used to adjust the seed for the structured values, and, as a result, produce better pseudo-random values?
Cogen
is used to generate arbitrary lambdas e.g. Arbitrary[A => B]
. If you have Cogen[A]
and Gen[B]
then you can chain A => Seed
, Seed => Seed
, and Seed => B
to get an arbitrary but deterministic A => B
function.
I think your implementation looks good, typically I try to avoid the manual plumbing of seeds and instead transform to some datatype for which Cogen
already exists. In this case that would be very messy 😁 but it could look something like this (basically an Either
encoding of a sum type).
Either[
Boolean,
Either[
Double,
Either[
String,
Either[
Long,
Either[...]
]
]
]
]
Motivation
.toString
for debugging purposes (the format is identical to open-telemetry java)sdk-trace
module #325 are represented assealed trait
s.Hash
andShow
are used to calculate hash and toString for such data types