-
Notifications
You must be signed in to change notification settings - Fork 35
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
trace sdk: add StatusData
#360
Conversation
iRevive
commented
Nov 10, 2023
Reference | Link |
---|---|
Spec | https://opentelemetry.io/docs/specs/otel/trace/api/#set-status |
Java implementation | StatusData.java |
291cb56
to
09516a9
Compare
The spec says:
Should we encode sealed abstract class StatusData(val code: StatusCode, description: Option[String])
object StatusData {
case object Ok extends StatusData(StatusCode.Ok, None)
case object Unset extends StatusData(StatusCode.Unset, None)
final case class Error(desc: Option[String]) extends StatusData(StatusCode.Error, desc)
} |
Sure, I might do something like this. sealed abstract class StatusData(val code: StatusCode) {
def description: Option[String]
}
object StatusData {
case object Ok extends StatusData(StatusCode.Ok) {
def description: None.type = None
}
case object Unset extends StatusData(StatusCode.Unset) {
def description: None.type = None
}
final case class Error(description: Option[String]) extends StatusData(StatusCode.Error)
} |
a9482fa
to
1f28bc4
Compare
* the description of the [[StatusData]]. Effective only for the | ||
* [[org.typelevel.otel4s.trace.Status.Error Status.Error]]. | ||
*/ | ||
def apply(status: Status, description: String): StatusData = |
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.
These apply
methods will be needed there:
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.
Technically, we can move them to the SdkSpanBackend
ab1bae0
to
1f28bc4
Compare
Hash.by(a => (a.status, a.description)) | ||
|
||
implicit val statusDataShow: Show[StatusData] = Show.show { data => | ||
show"StatusData{status=${data.status}, description=${data.description}}" |
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.
would it be better to leave out description
entirely if it's None
?
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 agree, thanks
test("use const instances when the given description is empty") { | ||
Prop.forAll(statusGen) { status => | ||
val expected = status match { | ||
case Status.Ok => StatusData.Ok | ||
case Status.Error => StatusData.Error(None) | ||
case Status.Unset => StatusData.Unset | ||
} | ||
|
||
assertEquals(StatusData(status), expected) | ||
assertEquals(StatusData(status, ""), expected) |
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 isn't really const for Error
, and isn't testing that the other two are constant (should assert(StatusData(...) eq expected)
)
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 renamed the test
1f28bc4
to
0d03a0d
Compare
0d03a0d
to
c1359d6
Compare