Make `SpanContext` sealed #359

iRevive · 2023-11-10T11:38:16Z

Reference	Link
Spec	https://opentelemetry.io/docs/specs/otel/trace/api/#spancontext
Java implementation	SpanContext.java

Motivation

Once the trait is sealed, we are in control of the implementations. And we can define a safe Hash instance.

otel4s-java-trace uses OpenTelemetry Java SDK under the hood.

Currently, we need to convert SpanContext to JSpanContext in 3 cases:

When SpanBuilder has links: e.g. Tracer[F].spanBuilder("child").addLink(otherSpan.context)
When we provide an explicit parent: Tracer[F].spanBuilder("child").withParent(otherSpan.context)
When we use a child scope: Tracer[F].childScope(otherSpan.context)

I've experimented with delegated instance too, see SpanContext and WrappedSpanContext.

However, Arman raised a valid concern that this approach violates the specification:

Warning

The API MUST implement methods to create a SpanContext. These methods SHOULD be the only way to create a SpanContext. This functionality MUST be fully implemented in the API, and SHOULD NOT be overridable

Considering that we need to convert SpanContext to JSpanContext rarely, in my opinion, we don't need a delegate instance.

WDYT?

armanbilge · 2023-11-10T17:07:43Z

sealing it is the first breaking API change since 0.3.0 was released (🎉) so we need to bump the base version.

otel4s/build.sbt

Line 3 in 762a460

ThisBuild / tlBaseVersion := "0.3"

armanbilge · 2023-11-10T17:37:14Z

java/trace/src/main/scala/org/typelevel/otel4s/java/trace/WrappedSpanContext.scala

-    TraceFlags.fromByte(jSpanContext.getTraceFlags.asByte)
-
-  lazy val traceState: TraceState = {
+  def wrap(context: JSpanContext): SpanContext = {


Nit: is wrap the correct terminology anymore?

We can rename WrappedSpanContextto SpanContextConversion?

object SpanContextConversion { def toJSpanContext(ctx: SpanContext): JSpanContext def toSpanContext(ctx: JSpanContext): SpanContext // or fromJSpanContext }

armanbilge · 2023-11-10T17:38:35Z

java/trace/src/main/scala/org/typelevel/otel4s/java/trace/WrappedSpanContext.scala

+    context.getTraceState.forEach((k, v) => entries.addOne(k -> v))
+    val traceState = TraceState.fromVectorUnsafe(entries.result())
+
+    SpanContext(


This is re-validating it right? Probably not a big deal but I suppose we could use some sort of unsafe constructor to skip validation in this specific case.

Yes, it will re-validate inputs. We can use SpanContext.createInternal instead.

iRevive · 2023-11-10T17:52:47Z

sealing it is the first breaking API change since 0.3.0 was released (🎉) so we need to bump the base version.

otel4s/build.sbt

Line 3 in 762a460

ThisBuild / tlBaseVersion := "0.3"

I forgot. It's a breaking API change, indeed. My disappointment is immeasurable and my day is ruined.

It means once we merge the PR, we cannot release patches (e.g. bugfix) for 0.3 anymore.

Should we make two 0.3.x and 0.4.x series-branches? But I have a feeling it's kinda an overkill for our adaptation level.

armanbilge · 2023-11-10T17:56:41Z

If the need to publish bug fixes arises, we can cut a series/0.3 branch from the v0.3.0 tag. Meanwhile we can continue using main for breaking development.

Edit: another option if there are bug fixes to release is to just declare v0.4.0, depending on the state of main.

armanbilge

LGTM but I look forward to finding out what I missed when April takes a look 😅

NthPortal

no significant concerns—just a few nitpicks and some bikeshedding

NthPortal · 2023-11-13T17:20:59Z

java/trace/src/main/scala/org/typelevel/otel4s/java/trace/SpanContextConversion.scala

+import org.typelevel.otel4s.trace.TraceState
+import scodec.bits.ByteVector
+
+private[otel4s] object SpanContextConversion {


should probably be called SpanContextConversions (plural)

NthPortal · 2023-11-13T17:23:50Z

java/trace/src/main/scala/org/typelevel/otel4s/java/trace/SpanContextConversion.scala

+
+private[otel4s] object SpanContextConversion {
+
+  def fromJSpanContext(context: JSpanContext): SpanContext = {


thoughts on toScala for this one and toJava for the other? a bit shorter name-wise and consistent with what the stdlib does for its conversions.

how often will end users need to use these conversions? if it's not super rare, extension methods would probably be a nice touch, though I'm happy to handle that if it ends up being needed

Much better than fromJSpanContext and toJSpanContext, thanks!

NthPortal · 2023-11-13T17:25:42Z

core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanContext.scala

+        ctx.traceIdHex,
+        ctx.spanIdHex,


any particular reason to use the hex Strings for the Hash rather than the ByteVectors?

There is no Hash[ByteVector] instance, unfortunately.

Oh, we can inline a quickie one with Hash.fromUniversalHashCode(). I think instances might live in scodec-cats but not worth dragging that in ...

Is there any benefit of using traceIdHex vs traceId? Since we treat traceId as a 'primary' property, we should use it in hashing too, right?

Since we treat traceId as a 'primary' property, we should use it in hashing too, right?

That's my thinking as well.

NthPortal · 2023-11-13T17:26:35Z

core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanContext.scala

+      isValid = isValid
+    )
+
+  private final case class SpanContextImpl(


extreme nitpick: should this just be called Impl, as it's already inside the SpanContext companion?

NthPortal · 2023-11-13T17:33:27Z

I look forward to finding out what I missed when April takes a look

if it doesn't have to do with the collections or at least the stdlib, usually nothing (as demonstrated)

# Conflicts: # java/trace/src/main/scala/org/typelevel/otel4s/java/trace/WrappedSpanContext.scala

iRevive added the tracing Improvements to tracing module label Nov 10, 2023

iRevive requested review from armanbilge and NthPortal November 10, 2023 11:38

Make SpanContext sealed

762a460

iRevive force-pushed the sdk-trace/sealed-span-context branch from 9c58514 to 762a460 Compare November 10, 2023 11:50

armanbilge reviewed Nov 10, 2023

View reviewed changes

Bump tlBaseVersion to 0.4

bdb12be

iRevive force-pushed the sdk-trace/sealed-span-context branch from 770d804 to dc2f0ed Compare November 10, 2023 18:18

Rename WrappedSpanContext to SpanContextConversion

10f003f

iRevive force-pushed the sdk-trace/sealed-span-context branch from dc2f0ed to 10f003f Compare November 10, 2023 18:19

iRevive requested a review from armanbilge November 10, 2023 18:26

armanbilge approved these changes Nov 10, 2023

View reviewed changes

NthPortal approved these changes Nov 13, 2023

View reviewed changes

iRevive added 2 commits November 13, 2023 20:26

Merge branch 'upstream-main' into sdk-trace/sealed-span-context

1fd0687

# Conflicts: # java/trace/src/main/scala/org/typelevel/otel4s/java/trace/WrappedSpanContext.scala

SpanContext: address feedback

2a1f332

iRevive force-pushed the sdk-trace/sealed-span-context branch from a2c1349 to 2a1f332 Compare November 13, 2023 18:32

Use traceId and spanId to calculate the SpanContext hash

4157b17

armanbilge approved these changes Nov 13, 2023

View reviewed changes

iRevive merged commit 88d43bb into typelevel:main Nov 13, 2023
10 checks passed

iRevive deleted the sdk-trace/sealed-span-context branch November 13, 2023 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `SpanContext` sealed #359

Make `SpanContext` sealed #359

iRevive commented Nov 10, 2023

armanbilge commented Nov 10, 2023

armanbilge Nov 10, 2023

iRevive Nov 10, 2023

armanbilge Nov 10, 2023

iRevive Nov 10, 2023 •

edited

Loading

iRevive commented Nov 10, 2023 •

edited

Loading

armanbilge commented Nov 10, 2023 •

edited

Loading

armanbilge left a comment

NthPortal left a comment

NthPortal Nov 13, 2023

NthPortal Nov 13, 2023

iRevive Nov 13, 2023

NthPortal Nov 13, 2023

iRevive Nov 13, 2023

armanbilge Nov 13, 2023

iRevive Nov 13, 2023

armanbilge Nov 13, 2023

NthPortal Nov 13, 2023

NthPortal commented Nov 13, 2023


		private[otel4s] object SpanContextConversion {

		def fromJSpanContext(context: JSpanContext): SpanContext = {

Make SpanContext sealed #359

Make SpanContext sealed #359

Conversation

iRevive commented Nov 10, 2023

Motivation

armanbilge commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

iRevive commented Nov 10, 2023 • edited Loading

armanbilge commented Nov 10, 2023 • edited Loading

armanbilge left a comment

Choose a reason for hiding this comment

NthPortal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthPortal commented Nov 13, 2023

Make `SpanContext` sealed #359

Make `SpanContext` sealed #359

iRevive Nov 10, 2023 •

edited

Loading

iRevive commented Nov 10, 2023 •

edited

Loading

armanbilge commented Nov 10, 2023 •

edited

Loading