Define tracing API #36

iRevive · 2022-07-29T17:54:21Z

The point of the PR is to describe the tracing API and simplify the review process. The API mimics OpenTelemetry with slight differences

iRevive · 2022-07-29T17:55:12Z

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

+    *   the set of attributes to associate with the link
+    */
+  def addLink(
+      spanContext: SpanContext,


~~Technically, Span[F] can be used here~~. No, it should be SpanContext otherwise an external context cannot be linked.

armanbilge · 2022-07-29T18:04:37Z

build.sbt

@@ -33,6 +33,7 @@ lazy val core = crossProject(JVMPlatform, JSPlatform)
    libraryDependencies ++= Seq(
      "org.typelevel" %%% "cats-core" % "2.7.0",
      "org.typelevel" %%% "cats-effect" % "3.3.13",
+      "co.fs2" %%% "fs2-core" % "3.2.11",


If you only need ByteVector you can include scodec-bits directly. I just mentioned its in fs2 as a proof of stability/wide adoption 😉

Highly likely we will need fs2 in the classpath to provide a proper tracing of streams.

iRevive · 2022-07-29T18:07:07Z

@janstenpickle could you have a look, please?

iRevive · 2022-07-29T18:11:36Z

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

+  /** Returns the trace identifier associated with this [[SpanContext]] as
+    * 16-byte vector.
+    */
+  def getTraceIdBytes: ByteVector


The method name claims it returns an array of bytes but it returns a ByteVector 🤔

What if we tweak names?

def getTraceId: ByteVector def getTraceIdHex: String

janstenpickle · 2022-07-29T19:13:09Z

@janstenpickle could you have a look, please?

Thanks, will do. Do you mind if I look on Monday? I've got quite a busy weekend 😅

iRevive · 2022-07-29T19:13:56Z

@janstenpickle could you have a look, please?

Thanks, will do. Do you mind if I look on Monday? I've got quite a busy weekend 😅

All good, there is no rush

janstenpickle

@iRevive I've added some initial thoughts!

janstenpickle · 2022-07-31T13:25:48Z

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

+    *
+    * Returns `None` if the span is invalid or no-op.
+    */
+  def context: Option[SpanContext]


In t4c we have an invalid instance of SpanContext. I think I like using an Option here better, but we might find that the Java implementation needs an invalid instance

Yeah, I'm scratching my head about this topic.

Official spec https://opentelemetry.io/docs/reference/specification/compatibility/opentracing/#get-the-active-span.

According to the OpenTelemetry specification, the invalid context can be described as

SpanContext { traceId = "00000000000000000000000000000000", spanId = "0000000000000000" }

If I get it right, invalid and noop spans are identical from the OpenTelemetry perspective. They should not produce child spans at all.

I see three states of the SpanContext:

Valid

Invalid (i.e. cannot create a span from remote traceId and spanId)

Noop

Perhaps it's better to stick with OpenTelemetry design and always return SpanContext because, well, the context is always there:

def context: SpanContext

Then a library user should verify the validity of the context if necessary.

While I like the Option, you're right it's probably a good idea to stay close to the spec.

Then a library user should verify the validity of the context if necessary.

This hasn't hurt us in t4c as far as I'm aware. No one has needed to differentiate between a no-op and invalid context.

janstenpickle · 2022-07-31T13:36:53Z

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

+trait SpanContext {
+
+  /** Returns the trace identifier associated with this [[SpanContext]] as
+    * 16-byte vector.
+    */
+  def traceId: ByteVector
+
+  /** Returns the trace identifier associated with this [[SpanContext]] as 32
+    * character lowercase hex String.
+    */
+  def traceIdHex: String
+
+  /** Returns the span identifier associated with this [[SpanContext]] as 8-byte
+    * vector.
+    */
+  def spanId: ByteVector
+
+  /** Returns the span identifier associated with this [[SpanContext]] as 16
+    * character lowercase hex String.
+    */
+  def spanIdHex: String
+
+  /** Returns the sampling strategy of this [[SpanContext]]. Indicates whether
+    * the span in this context is sampled.
+    */
+  def samplingDecision: SamplingDecision
+
+  /** Returns `true` if this [[SpanContext]] is valid.
+    */
+  def isValid: Boolean
+
+  /** Returns `true` if this [[SpanContext]] was propagated from a remote
+    * parent.
+    */
+  def isRemote: Boolean
+}


I think it would be a good idea to make this sealed to provde "safe" and "unsafe" constructors for SpanContexts with ByteVectors. There should probably also be a constructor that safely generates a context from Random.

This might make isValid redundant of course. I suppose the question is do we want to pass through invalid contexts parsed/converted from upstream system and whether we want to differentiate between a no-op and invalid data. My hunch is that it's probably a good idea to represent a no-op as a None and invalid as false here, to aid debugging.

I was thinking about a simple wrapper for java implementation.

Safe and unsafe constructors make sense to me. They are useful for third-party integrations (i.e. creating SpanContext from HTTP request headers). I will add them.

janstenpickle · 2022-07-31T13:38:35Z

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

+object SpanFinalizer {
+
+  type Strategy = PartialFunction[Resource.ExitCase, SpanFinalizer]
+
+  object Strategy {
+    def empty: Strategy = PartialFunction.empty
+
+    def reportAbnormal: Strategy = {
+      case Resource.ExitCase.Errored(e) =>
+        SpanFinalizer.multiple(
+          SpanFinalizer.RecordException(e),
+          SpanFinalizer.SetStatus(Status.Error, None)
+        )
+
+      case Resource.ExitCase.Canceled =>
+        SpanFinalizer.SetStatus(Status.Error, Some("canceled"))
+    }
+  }
+
+  final case class RecordException(throwable: Throwable) extends SpanFinalizer
+
+  final case class SetStatus(status: Status, description: Option[String])
+      extends SpanFinalizer
+
+  final case class SetAttributes(attributes: Seq[Attribute[_]])
+      extends SpanFinalizer
+
+  final case class Multiple(finalizers: NonEmptyList[SpanFinalizer])
+      extends SpanFinalizer
+
+  def multiple(head: SpanFinalizer, tail: SpanFinalizer*): Multiple =
+    Multiple(NonEmptyList.of(head, tail: _*))
+
+}


iRevive · 2022-08-01T07:57:10Z

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

+    }
+  }
+
+  final case class RecordException(throwable: Throwable) extends SpanFinalizer


Should the ~~case~~ classes be hidden? Is it less painful in terms of bincompat?

private final class RecordException(...) extends SpanFinalizer def recordException(throwable: Throwable): SpanFinalizer = RecordException(throwable)

If you avoid the case and make the constructor private it should be fine. Unless in the future you may want to remove the RecordException type, then better not to expose it.

# Conflicts: # build.sbt

zmccoy · 2022-08-04T18:53:47Z

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

+
+/** A decision on whether a span should be recorded or dropped.
+  */
+sealed abstract class SamplingDecision(val isSampled: Boolean)


In the Java client there is a RECORD_ONLY type as well for client based sampling it looks like. Is that something we wanted to have here? https://github.com/open-telemetry/opentelemetry-java/blob/50408d499f85d5761d0a5ed9bf9d77d5ff01fff5/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/samplers/SamplingDecision.java#L17

I would like to mirror all types, but I do not understand how to construct the proper type since the input type is Boolean:

https://github.com/open-telemetry/opentelemetry-java/blob/86eea94b7695aba4a871e04f042fd0c0e84f607f/api/all/src/main/java/io/opentelemetry/api/trace/SpanContext.java#L126

OpenTelemetry calculates SamplingDecision in internals:
https://github.com/open-telemetry/opentelemetry-java/blob/50408d499f85d5761d0a5ed9bf9d77d5ff01fff5/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpanBuilder.java#L190-L195

I'll take a look at those.

zmccoy · 2022-08-05T15:37:27Z

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

+    *
+    * @param attributes
+    *   the set of attributes to add to the span
+    */


This is getting pretty specific to the spec, but the Otel spec requires the API to provide a single attribute setter, and then optionally a setter for multiple attributes at once. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-attributes

Would it make sense to make two different methods to separate those use cases out rather than just a vararg solution for both?

Good question. I'm fine with varargs.
span.setAttributes(Attribute(...), Attribute(...)) is handier than span.setAttributes(Seq(Attribute(...), Attribute(...)))

We still can add def setAttribute[A](attribute: Attribute[A]): F[Unit], it will not hurt for sure.

Yeah I think adding setAttribute just to fulfill the requirement and make it easy to communicate about api shapes to other folks using Otel would be great.

I added the method

Thanks! Looks great.

# Conflicts: # build.sbt

iRevive · 2022-09-25T13:17:46Z

@rossabaker could you review this one, please?

rossabaker

This is fantastic work.

core/src/main/scala/org/typelevel/otel4s/meta/InstrumentMeta.scala

core/tracing/src/main/scala/org/typelevel/otel4s/trace/Span.scala

rossabaker · 2022-09-29T13:32:21Z

core/tracing/src/main/scala/org/typelevel/otel4s/trace/SpanBuilder.scala

+    * @see
+    *   default finalization strategy [[SpanFinalizer.Strategy.reportAbnormal]]
+    */
+  def createManual: Resource[F, Span.Manual[F]]


Why is this a Resource instead of an F[Span.Manual[F]]?

Does this even need to exist at all? Is it different from createAuto.allocated? Only thing I can think of is that it gives us a named end.

Resource is needed to apply the finalization strategy during release.

otel4s/core/tracing/src/main/scala/org/typelevel/otel4s/trace/SpanBuilder.scala

Line 92 in f2db47d

def withFinalizationStrategy(strategy: SpanFinalizer.Strategy): SpanBuilder[F]

Also, a manual span is not ended upon resource release. This should be done manually.

The question is the following: do we have a scenario where a span should outlive the scope and be ended somewhere after? Nothing comes to my head right away.

We can do the following:

Replace Span.Auto and Span.Manual with Span

Add the end method to Span

Rename SpanBuilder#createAuto to create

That way it is possible to end a span earlier.

After end,

Implementations SHOULD ignore all subsequent calls to End and any other Span methods, i.e. the Span becomes non-recording by being ended

For the finalizers to be useful, they need to be sequenced before end. And I don't know why we'd want to call the finalizers, allow other work, and then end. If that's all true, I think all we need is a resource, and "manual" is just an allocated resource.

What gives me pause is that in the spec, end can take:

(Optional) Timestamp to explicitly set the end timestamp.

I like the idea of hiding end, because it changes the state of the span and what's still legal on it, and embedding it in the resource finalizer makes it clear where this happens. But I'm not sure how to do that and support the explicit end.

What if we had end that triggered the finalizer? "Manual" just returns an F[Span[F]], and automatic returns a Resource[F, Span[F]] that invokes end? Then there's still just one kind of Span.

Sounds reasonable. There is no sense in invoking finalizers when the span has been terminated. I updated API definitions.

core/tracing/src/main/scala/org/typelevel/otel4s/trace/Tracer.scala

# Conflicts: # build.sbt

rossabaker

This looks good to me!

iRevive commented Jul 29, 2022

View reviewed changes

Define tracing API

ff8ed12

iRevive force-pushed the tracing-api branch from 8414663 to ff8ed12 Compare July 29, 2022 18:01

armanbilge reviewed Jul 29, 2022

View reviewed changes

iRevive commented Jul 29, 2022

View reviewed changes

iRevive added 4 commits July 30, 2022 11:19

Add spanBuilder to Tracer. Better naming of SpanContext members

9f05624

Rename SamplingStrategy -> SamplingDecision

e77a192

Update scaladoc. Add SpanFinalizer

9ab5d68

Fix scaladoc

42cdd4b

This was referenced Jul 30, 2022

Implement tracing API #37

Merged

[Do not merge] Trace API playground #35

Closed

iRevive requested a review from rossabaker July 31, 2022 08:24

iRevive added 2 commits July 31, 2022 12:10

Improve Scaladoc

a7a390f

Better phrasing

bbe21d7

janstenpickle reviewed Jul 31, 2022

View reviewed changes

iRevive commented Aug 1, 2022

View reviewed changes

iRevive added 6 commits August 1, 2022 13:12

Hide implementation of SpanFinalizer

8f74413

Docs update

5fdb830

Add withParent to SpanBuilder

13d4ebb

Replace childOf with childScope in Tracer

6d3f38f

Make Span always have a SpanContext

3a14e52

Merge branch 'main' into tracing-api

3d51acb

# Conflicts: # build.sbt

zmccoy reviewed Aug 4, 2022

View reviewed changes

zmccoy reviewed Aug 5, 2022

View reviewed changes

Add setAttribute method to Span

b1d4b15

zmccoy approved these changes Aug 11, 2022

View reviewed changes

iRevive mentioned this pull request Aug 11, 2022

Split project into modules #39

Merged

iRevive added 4 commits September 25, 2022 14:26

Merge branch 'main' into tracing-api

a9b5f30

# Conflicts: # build.sbt

Update layout

39ee6eb

Run sbt githubWorkflowGenerate

bb99d2a

Revert redundant changes

f2db47d

rossabaker reviewed Sep 29, 2022

View reviewed changes

iRevive added 3 commits September 30, 2022 10:40

Merge branch 'main' into tracing-api

4a20264

Make createManual return F[Span[F]]

90dd256

Merge branch 'main' into tracing-api

6e19a25

# Conflicts: # build.sbt

iRevive requested a review from rossabaker October 17, 2022 09:27

rossabaker approved these changes Oct 17, 2022

View reviewed changes

iRevive merged commit 6cabd1b into typelevel:main Oct 17, 2022

armanbilge mentioned this pull request Jun 7, 2023

Creating SpanContext + adherence to OTel specs #243

Closed

chr12c pushed a commit to chr12c/otel4s that referenced this pull request Jun 26, 2023

Define tracing API (typelevel#36)

15a249e

iRevive deleted the tracing-api branch November 29, 2024 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define tracing API #36

Define tracing API #36

iRevive commented Jul 29, 2022 •

edited

Loading

iRevive Jul 29, 2022 •

edited

Loading

armanbilge Jul 29, 2022

iRevive Jul 29, 2022

iRevive commented Jul 29, 2022

iRevive Jul 29, 2022

janstenpickle commented Jul 29, 2022

iRevive commented Jul 29, 2022

janstenpickle left a comment

janstenpickle Jul 31, 2022

iRevive Jul 31, 2022

janstenpickle Jul 31, 2022

janstenpickle Jul 31, 2022

iRevive Aug 2, 2022

janstenpickle Jul 31, 2022

iRevive Aug 1, 2022 •

edited

Loading

armanbilge Aug 1, 2022

zmccoy Aug 4, 2022

iRevive Aug 5, 2022

zmccoy Aug 5, 2022

zmccoy Aug 5, 2022 •

edited

Loading

iRevive Aug 6, 2022 •

edited

Loading

zmccoy Aug 8, 2022 •

edited

Loading

iRevive Aug 8, 2022

zmccoy Aug 8, 2022

iRevive commented Sep 25, 2022

rossabaker left a comment

rossabaker Sep 29, 2022

rossabaker Sep 29, 2022

iRevive Sep 29, 2022 •

edited

Loading

rossabaker Oct 17, 2022

iRevive Oct 17, 2022

rossabaker left a comment

Define tracing API #36

Define tracing API #36

Conversation

iRevive commented Jul 29, 2022 • edited Loading

iRevive Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive commented Jul 29, 2022

Choose a reason for hiding this comment

janstenpickle commented Jul 29, 2022

iRevive commented Jul 29, 2022

janstenpickle 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 Aug 1, 2022 • edited Loading

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

zmccoy Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

iRevive Aug 6, 2022 • edited Loading

Choose a reason for hiding this comment

zmccoy Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive commented Sep 25, 2022

rossabaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossabaker left a comment

Choose a reason for hiding this comment

iRevive commented Jul 29, 2022 •

edited

Loading

iRevive Jul 29, 2022 •

edited

Loading

iRevive Aug 1, 2022 •

edited

Loading

zmccoy Aug 5, 2022 •

edited

Loading

iRevive Aug 6, 2022 •

edited

Loading

zmccoy Aug 8, 2022 •

edited

Loading

iRevive Sep 29, 2022 •

edited

Loading