New modules + span context maybe? #236

sherriesyt · 2023-06-05T02:14:01Z

Draft PR as usual! New modules added to the build file.

Feedback welcome; I took a guess at what to do here (and a very big guess at possible dependencies, though, I'll know as I go?)

I also figured "add a new module to the build file" and "implement my first thing (span context)?" could go hand-in-hand for my first PR, because with some implementation, I'll better know some of those dependencies (and also because the former, by itself, felt a bit small for one PR. Though let me know if we should get that merged first anyway, and I'll separate the concerns).

armanbilge

👍 module setup looks good!

armanbilge · 2023-06-05T02:21:07Z

build.sbt

+lazy val `sdk-common` = crossProject(JVMPlatform, JSPlatform, NativePlatform)
+  .crossType(CrossType.Pure)
+  .in(file("sdk/common"))
+  .dependsOn(`core-common`)
+  .settings(
+    name := "otel4s-sdk-common",
+    libraryDependencies ++= Seq(
+      "org.typelevel" %%% "cats-effect-kernel" % CatsEffectVersion,
+      "org.typelevel" %%% "cats-mtl" % CatsMtlVersion,
+      "org.scalameta" %%% "munit" % MUnitVersion % Test
+    ),
+  )
+
+lazy val `sdk-trace` = crossProject(JVMPlatform, JSPlatform, NativePlatform)
+  .crossType(CrossType.Pure)
+  .in(file("sdk/trace"))
+  .dependsOn(`sdk-common`, `core-trace`)
+  .settings(
+    name := "otel4s-sdk-trace",
+    libraryDependencies ++= Seq(
+      "org.typelevel" %%% "cats-effect" % CatsEffectVersion,
+    ),
+  )


Probably we can add munitDependencies to both modules. You are planning to write tests for tracing right 😜

Suggested change

lazy val `sdk-common` = crossProject(JVMPlatform, JSPlatform, NativePlatform)

.crossType(CrossType.Pure)

.in(file("sdk/common"))

.dependsOn(`core-common`)

.settings(

name := "otel4s-sdk-common",

libraryDependencies ++= Seq(

"org.typelevel" %%% "cats-effect-kernel" % CatsEffectVersion,

"org.typelevel" %%% "cats-mtl" % CatsMtlVersion,

"org.scalameta" %%% "munit" % MUnitVersion % Test

),

)

lazy val `sdk-trace` = crossProject(JVMPlatform, JSPlatform, NativePlatform)

.crossType(CrossType.Pure)

.in(file("sdk/trace"))

.dependsOn(`sdk-common`, `core-trace`)

.settings(

name := "otel4s-sdk-trace",

libraryDependencies ++= Seq(

"org.typelevel" %%% "cats-effect" % CatsEffectVersion,

),

)

lazy val `sdk-common` = crossProject(JVMPlatform, JSPlatform, NativePlatform)

.crossType(CrossType.Pure)

.in(file("sdk/common"))

.dependsOn(`core-common`)

.settings(

name := "otel4s-sdk-common",

libraryDependencies ++= Seq(

"org.typelevel" %%% "cats-effect-kernel" % CatsEffectVersion,

"org.typelevel" %%% "cats-mtl" % CatsMtlVersion

),

)

.settings(munitDependencies)

lazy val `sdk-trace` = crossProject(JVMPlatform, JSPlatform, NativePlatform)

.crossType(CrossType.Pure)

.in(file("sdk/trace"))

.dependsOn(`sdk-common`, `core-trace`)

.settings(

name := "otel4s-sdk-trace",

libraryDependencies ++= Seq(

"org.typelevel" %%% "cats-effect" % CatsEffectVersion,

),

)

.settings(munitDependencies)

Oops, just saw this. Will revise to add :)

sherriesyt · 2023-06-05T04:39:38Z

I wanted to clarify -- is an implementation of SpanContext supposed to sit in our new SDK? I thought "yes," at first, but I did find this line in the OTel specs for a span context, so I wanted to clarify:

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.

Is the implementation instead going to sit in our existing SpanContext.scala file? I see that right now we have the creation of an "invalid" SpanContext there, but I'm wondering where else we're meant to implement the SpanContext trait (in the SDK, in an existing file, elsewhere?) now.

armanbilge · 2023-06-05T04:39:41Z

sdk/trace/src/main/scala/org/typelevel/otel4s/SpanContextImpl.scala

+ * limitations under the License.
+ */
+
+package org.typelevel.otel4s // ?


I think this would be the appropriate package name?

Suggested change

package org.typelevel.otel4s // ?

package org.typelevel.otel4s.sdk.trace

armanbilge · 2023-06-05T04:42:50Z

sdk/trace/src/main/scala/org/typelevel/otel4s/SpanContextImpl.scala

+
+import org.typelevel.otel4s.trace.SpanContext
+
+final class SpanContextImpl extends SpanContext { // does a impl belong in the sdk?


I think we can make this a case class with all the fields below as its members. And actually we can probably keep the class private, since users will always work with the SpanContext interface.

armanbilge · 2023-06-05T04:58:21Z

Ohhhh, very good catch 🤔

I wanted to clarify -- is an implementation of SpanContext supposed to sit in our new SDK? I thought "yes," at first, but I did find this line in the OTel specs for a span context, so I wanted to clarify:

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.

That does sound like it should be implemented in the API, and not in the SDK. That seems fine for the Scala SDK, but will it complicate the Java wrapper?

sherriesyt · 2023-06-07T14:05:44Z

Wondering if some work here rightfully belongs in #243 instead. Will make new PR and shift some changes over there, if that's appropriate

armanbilge · 2023-06-07T14:07:36Z

Will make new PR and shift some changes over there, if that's appropriate

👍 yes definitely, I think the SpanContextImpl work should go in the API modules and target the main branch. Thanks!

armanbilge · 2023-06-08T18:36:46Z

build.sbt

+  )
+  .settings(munitDependencies)
+
+lazy val `sdk-trace` = crossProject(JVMPlatform, JSPlatform, NativePlatform)


Oh I forgot, you also need to add these to the "root aggregate" somewhere higher up in the build. I think that's why CI appears to be passing, because it's not actually compiling these sources yet 😅

iRevive · 2023-06-09T07:29:41Z

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

+    samplingDecision: SamplingDecision,
+    isValid: Boolean,
+    isRemote: Boolean,
+    storeInContext: Vault


SpanContext defines storeInContext as follows:

def storeInContext(context: Vault): Vault = context.insert(SpanContext.key, this)

Should we make it final and remove it from the SpanContextImpl? I guess it should work unless there will be a custom logic.

armanbilge · 2023-06-14T04:18:17Z

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

+  def samplingDecision: SamplingDecision =
+    SamplingDecision.fromBoolean(
+      (flags & SpanContext.SampledMask) == SpanContext.SampledMask
+    )


@iRevive I'm a little confused if this is the right API to expose here.

I see SamplingDecision is described here, which is actually an enum with three states.
https://opentelemetry.io/docs/specs/otel/trace/sdk/#shouldsample

But this seems to be different from the Sampled flag, which is only a Boolean and is described here.
https://opentelemetry.io/docs/specs/otel/trace/sdk/#sampling

I see that the Java API exposes a Boolean on SpanContext, even though they also have a SamplingDecision enum.

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

Lines 42 to 43 in 6bb372d

lazy val samplingDecision: SamplingDecision =

SamplingDecision.fromBoolean(jSpanContext.isSampled)

…g WrappedSpanContext

…-context

armanbilge

Nearly there! Just needs a couple tests, some bike shed, some polish, and a fight with CI 😅

armanbilge · 2023-07-10T19:42:34Z

build.sbt

+      "org.typelevel" %%% "cats-mtl" % CatsMtlVersion,
+    ),
+  )
+  .settings(munitDependencies)


Let's add .enablePlugins(NoPublishPlugin) for sdk-common and sdk-trace until we are ready to publish them.

armanbilge · 2023-07-10T19:49:39Z

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

+  def isValidSpanId(id: ByteVector): Boolean =
+    (id.length == SpanContext.SpanId.HexLength) && (id != SpanContext.invalid.spanId)
+
+  /** Checks whether a trace id has correct length and is not the
+    * invalid id.
+    */
+  def isValidTraceId(id: ByteVector): Boolean =
+    (id.length == SpanContext.TraceId.HexLength) && (id != SpanContext.invalid.traceId)


These are some good methods to have some tests for.

armanbilge · 2023-07-10T19:51:18Z

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

+
+sealed trait TraceFlags {
+  def toByte: Byte 
+  def isSampled: Boolean


I think isSampled is another good one to test. e.g. check that it's value is expected for fromByte(0) and fromByte(1) and maybe a couple other random values ...

armanbilge · 2023-07-10T19:59:10Z

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

-private[trace] object WrappedSpanContext {
+private[java] object SpanConversions {
+
+  def wrap(jSpanContext: JSpanContext): SpanContext = {


Maybe fromJSpanContext, toJSpanContext?

armanbilge · 2023-07-10T20:09:00Z

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

+    lazy val traceId: ByteVector =
+      ByteVector(jSpanContext.getTraceIdBytes)
+
+    lazy val spanId: ByteVector =
+      ByteVector(jSpanContext.getSpanIdBytes)
+
+    val traceFlags = TraceFlags.fromByte(jSpanContext.getTraceFlags().asByte())
+
+    def isRemote: Boolean =
+      jSpanContext.isRemote


probably these can all be vals now.

armanbilge · 2023-07-10T20:09:29Z

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

    context match {
-      case ctx: WrappedSpanContext =>
-        ctx.jSpanContext

      case other if other.isRemote =>


we no longer need a match, we can directly call methods on context.

# Conflicts: # .github/workflows/ci.yml # java/trace/src/main/scala/org/typelevel/otel4s/java/ContextConversions.scala

sherriesyt added 3 commits June 4, 2023 01:03

Add some new sdk projects and folders

e60ba3d

Fix sdk-common project

afce266

Ran githubWorkflowGenerate

a24aa7e

armanbilge reviewed Jun 5, 2023

View reviewed changes

sherriesyt added 2 commits June 5, 2023 00:20

Reorganize SDK folders, stub out SpanContextImpl?

2b647a9

Add munitDependencies to new modules

ef33292

armanbilge reviewed Jun 5, 2023

View reviewed changes

armanbilge mentioned this pull request Jun 5, 2023

Tracing SDK #234

Closed

9 tasks

sherriesyt mentioned this pull request Jun 7, 2023

Creating SpanContext + adherence to OTel specs #243

Closed

Put SpanContext case class in API

ab75c57

armanbilge reviewed Jun 8, 2023

View reviewed changes

iRevive reviewed Jun 9, 2023

View reviewed changes

sherriesyt added 4 commits June 12, 2023 00:12

Pull from main, finish merge

5a21215

Add new modules to aggregate

56c5502

Make storeInContext final, remove from impl

8e18a89

Modify SpanContext, add samplingDecision

e578f5b

armanbilge reviewed Jun 14, 2023

View reviewed changes

sherriesyt added 3 commits June 24, 2023 16:30

Delete SamplingDecision in API, expose SpanComtextImpl, begin revisin…

e3b215e

…g WrappedSpanContext

Add create method on SpanContext

5638a20

Merge branch 'main' of https://github.com/sherriesyt/otel4s into span…

05e0666

…-context

sherriesyt changed the base branch from sdk to main July 1, 2023 03:20

Refactor SpanContext and add TraceFlags wrapper

7cb8e5f

armanbilge reviewed Jul 10, 2023

View reviewed changes

armanbilge added this to the 0.3 milestone Jul 15, 2023

armanbilge mentioned this pull request Oct 1, 2023

Experimental sdk-trace module #325

Closed

9 tasks

iRevive mentioned this pull request Oct 2, 2023

Add traceFlags to SpanContext #326

Closed

iRevive added 2 commits October 3, 2023 15:33

Merge branch 'upstream-main' into span-context

c98f619

# Conflicts: # .github/workflows/ci.yml # java/trace/src/main/scala/org/typelevel/otel4s/java/ContextConversions.scala

Merge upstream, add docs and tests

4630dd3

iRevive marked this pull request as ready for review October 3, 2023 14:36

iRevive merged commit cc7e98a into typelevel:main Oct 3, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New modules + span context maybe? #236

New modules + span context maybe? #236

sherriesyt commented Jun 5, 2023

armanbilge left a comment

armanbilge Jun 5, 2023

sherriesyt Jun 5, 2023

sherriesyt commented Jun 5, 2023

armanbilge Jun 5, 2023

armanbilge Jun 5, 2023

armanbilge commented Jun 5, 2023

sherriesyt commented Jun 7, 2023

armanbilge commented Jun 7, 2023

armanbilge Jun 8, 2023

iRevive Jun 9, 2023 •

edited

Loading

armanbilge Jun 14, 2023

armanbilge left a comment

armanbilge Jul 10, 2023

armanbilge Jul 10, 2023

armanbilge Jul 10, 2023

armanbilge Jul 10, 2023

armanbilge Jul 10, 2023

armanbilge Jul 10, 2023

	package org.typelevel.otel4s // ?
	package org.typelevel.otel4s.sdk.trace


		import org.typelevel.otel4s.trace.SpanContext

		final class SpanContextImpl extends SpanContext { // does a impl belong in the sdk?

	lazy val samplingDecision: SamplingDecision =
	SamplingDecision.fromBoolean(jSpanContext.isSampled)

New modules + span context maybe? #236

New modules + span context maybe? #236

Conversation

sherriesyt commented Jun 5, 2023

armanbilge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sherriesyt commented Jun 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge commented Jun 5, 2023

sherriesyt commented Jun 7, 2023

armanbilge commented Jun 7, 2023

Choose a reason for hiding this comment

iRevive Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge 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

iRevive Jun 9, 2023 •

edited

Loading