sdk-trace: add `SpanExporter` #373

iRevive · 2023-11-15T10:35:26Z

Reference	Link
Spec	https://opentelemetry.io/docs/specs/otel/trace/sdk/#span-exporter
Java implementation	SpanExporter.java

1) The exporter MUST support three functions: Export, Shutdown, and ForceFlush.

Well, Export and ForceFlush are obvious. Shutdown, however, is more tricky.

Since the CE offers flexible lifecycle management of resources, I see two options:

Each SpanExporter should manage its lifecycle on its own:

def otlpHttpExporter: Resource[F, SpanExporter[F]] = ???
def otlpGrpcExporter: Resource[F, SpanExporter[F]] = ???

val exporter: Resource[F, SpanEporter[F]] = 
  for {
    otlpHttp <- otlpHttpExporter
    otlpGrpc <- otlpGrpcExporter
  } yield SpanExporter.of(otlpHttp, otlpGrpc)

We make SpanExporter.of return a Resource[F, SpanExporter[F]] and the given exporters will be shutdown there:

def of[F[_]](exporters: SpanExporter[F]): Resource[F, SpanExporter[F]] = 
  Resource.make(...)(_ => exporters.traverse_(_.shutdown))

The same exporter will likely be shut down several times: by SpanExporter.of and by the exporter's lifecycle management (assuming the allocation of an exporter is defined as a Resource).

If we choose option 1, should we define a def shutdown: F[Unit] in the SpanExporter interface? This situation is similar to what we've discussed with April before #347.

2) `export` must return the `ExportResult`

Java SDK somewhat cheats here and returns CompletableResultCode. Which we can mimic as exporter.exportSpan(...).attempt.

Note

Returns: ExportResult:

The return of Export() is implementation specific. In what is idiomatic for the language the Exporter must send an > ExportResult to the Processor. ExportResult has values of either Success or Failure:

Success - The batch has been successfully exported. For protocol exporters this typically means that the data is sent over > the wire and delivered to the destination server.
Failure - exporting failed. The batch must be dropped. For example, this can happen when the batch contains bad data and > cannot be serialized.

We can always define it as an ADT:

sealed trait ExportResult
object ExportResult {
  case object Success extends ExportResult
  case class Failure(reason: String) extends ExportResult
}

So far so good, but if we use a Multi exporter, we need to combine results for different exporters and distinguish one from another.

sealed trait ExportResult
object ExportResult {
  case class ByExporter(exporter: String, result: ExportResult)
  
  case object Success extends ExportResult
  case class Failure(reason: String) extends ExportResult
  case class Compound(results: NonEmptyList[ByExporter]) extends ExportResult
  
  implicit val compoundSemigroup: Semigroup[Compound] =
    (x, y) => Compound(x.results.concatNel(y.results))
}

def exportSpans(span: List[SpanData]): F[ExportResult] = 
  exporters.reduceMapM { exporter =>
    for {
      result <- exporter.exportSpans(span)
    } yield ExportResult.Compound(NonEmptyList.one(ByExporter(exporter.toString, result)))
  }

In most cases, you are going to use one exporter. So is it worth the hustle?

sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/exporter/SpanExporter.scala

armanbilge · 2023-11-16T17:39:48Z

sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/exporter/SpanExporter.scala

+    def exportSpans(span: List[SpanData]): F[Unit] =
+      exporters.traverse_(_.exportSpans(span))
+
+    def flush: F[Unit] =
+      exporters.traverse_(_.flush)


I wonder if these should be parTraverse, so that exporting/flushing can happen concurrently.

I believe we can use parTraverse. Java SDK does something similar. They submit all tasks and then aggregate the results: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/export/MultiSpanExporter.java#L40-L55

armanbilge · 2023-11-21T09:50:09Z

sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/exporter/SpanExporter.scala

+  /** Creates a [[SpanExporter]] which delegates all exports to the exporters in
+    * order.


If we use Parallel then "in order" is not true anymore.

Thanks, I updated the documentation.

armanbilge · 2023-11-21T10:38:36Z

sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/exporter/SpanExporter.scala

+    * @param span
+    *   the collection of sampled Spans to be exported
+    */
+  def exportSpans(span: List[SpanData]): F[Unit]


Suggested change

def exportSpans(span: List[SpanData]): F[Unit]

def exportSpans(spans: List[SpanData]): F[Unit]

armanbilge · 2023-11-21T10:39:00Z

sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/exporter/SpanExporter.scala

+    * depending on the type of span processor being used. However, the batch
+    * span exporter will ensure that only one export can occur at a time.


Sorry, I'm confused by this comment. What is the "batch span exporter"?

Oh, it should be batch span processor instead. Once I add it, I will use a scaladoc reference (e.g. [[BatchSpanProcessor]].

The implementation will be based on:
https://github.com/typelevel/otel4s/pull/325/files#diff-ff368903b03e16d45e93659be2898e27f243b1a48dd4af8e516e9ee69048dbba

I've updated scaladoc

NthPortal

LGTM, though I'd prefer a wider input type than List

NthPortal · 2023-11-24T18:56:22Z

sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/exporter/SpanExporter.scala

+    * @param spans
+    *   the collection of sampled spans to be exported
+    */
+  def exportSpans(spans: List[SpanData]): F[Unit]


could this take immutable.Iterable instead of List? or at least Seq (although it's my impression that the SpanData objects are not expected to be meaningfully ordered)

Yeah. List is kinda strict. Seq is definitely fits better

If you don't want to hard-code List, then what about something like this.

Suggested change

def exportSpans(spans: List[SpanData]): F[Unit]

def exportSpans[G[_]: Foldable](spans: G[SpanData]): F[Unit]

the part of me that loves the collections framework prefers IterableOnce, but I am also okay with a Foldable context bound

Yeah, this is 💯 a matter of taste. I prefer Foldable to >: Seq, similar to how we use Sync[F] and don't have IO <: SyncIO.

NthPortal · 2023-11-24T19:02:46Z

sdk/trace/src/test/scala/org/typelevel/otel4s/sdk/trace/exporter/InMemorySpanExporter.scala

+
+// todo: should be in the testkit package
+final class InMemorySpanExporter[F[_]: Applicative] private (
+    storage: Ref[F, Chain[SpanData]]


I'm not a huge fan of Chain in general, as it (for no apparent reason) does not participate in the stdlib collections framework, or even a custom collections framework within cats.data. it's also unclear how its performance compares to a reasonable equivalent from the stdlib such as Queue; it's only compared to List and Vector in its docs. though it's hard to argue with how well it semantically fits repeated calls to exportSpans

What if we use Vector instead? For an (upcoming) testkit exporter, it should be enough

hmm. are you okay with Ref[F, Queue[SpanData]] and having finishedSpans return F[Seq[SpanData]]? I think Queue fits better than Vector here

armanbilge · 2023-11-24T19:54:30Z

sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/exporter/SpanExporter.scala

+      exporters: NonEmptyList[SpanExporter[F]]
+  ) extends SpanExporter[F] {
+    def exportSpans(spans: List[SpanData]): F[Unit] =
+      exporters.parTraverse_(_.exportSpans(spans))


Currently this fails fast. If any of the exporters fails, then the rest will be canceled. I'm not sure if that's the behavior we want.

We might want to use attempt and gather the failures into CompositeFailure, if there is more than one?

That makes sense.

How should we encode the CompositeFailure? Should we take exporter name into the account?

I've describe possible solutions in the description, see 2) export must return the ExportResult (GitHub doesn't allow ref link to the description 😞 )

I agree it's not worth the hustle /shrug the spec doesn't say anything about this right?

Let's say we have 10 exporters. 2 complete with failures and 8 work fine.
I don't think we should cut the other 8 healthy exporters off.
As you suggested, we can use attempt while evaluating each exportSpans(spans). And in the end, we can throw a compose failure (if any). The healthy exports will do their job, while the end user will still have enough details about the failures.
Well, it is kinda far away from being ideal or reasonable, but it still may work.

We can also consider using Ior type.

Oh, in case it wasn't clear, CompositeFailure is an FS2 thing.

https://www.javadoc.io/doc/co.fs2/fs2-docs_2.13/latest/fs2/CompositeFailure.html

Ah, I completely forgot. Thanks!

Hm, we need to bring fs2-core dependency then. I'm not sure if it's worth it for a single exception.

Aha, okay, I couldn't remember if FS2 was already a dependency of this module.

Eh. In that case I say we just raise the first exception or something, idk.

If we want to keep exporters as a separate module, fs2 dependency is not needed. If we want to have exporters as a part of the sdk module, we can incorporate fs2-core

I added a variation of the CompositeFailure.

iRevive · 2023-11-27T09:04:14Z

I realized that I made the review process complicated with the force-pushes. I should squash commits when we are done.

Sorry for that 😬

iRevive · 2023-11-27T09:05:47Z

I added a composite failure:

https://github.com/typelevel/otel4s/pull/373/files#diff-446dd47f23242e790aae05d7c55f011715b8fc49cdfc71444cbd2c7c0965c035R130-R150

And the error management:

https://github.com/typelevel/otel4s/pull/373/files#diff-446dd47f23242e790aae05d7c55f011715b8fc49cdfc71444cbd2c7c0965c035R168-R198

iRevive added tracing Improvements to tracing module module:sdk Features and improvements to the sdk module labels Nov 15, 2023

iRevive requested review from armanbilge and NthPortal November 15, 2023 10:35

iRevive force-pushed the sdk-trace/span-exporter branch from 45e2335 to ff378ea Compare November 15, 2023 18:22

iRevive marked this pull request as ready for review November 15, 2023 18:22

iRevive force-pushed the sdk-trace/span-exporter branch from ff378ea to 37764f4 Compare November 15, 2023 18:46

armanbilge reviewed Nov 15, 2023

View reviewed changes

sdk/trace/src/main/scala/org/typelevel/otel4s/sdk/trace/exporter/SpanExporter.scala Outdated Show resolved Hide resolved

iRevive force-pushed the sdk-trace/span-exporter branch 2 times, most recently from 21ac4fb to f1ce3fe Compare November 16, 2023 10:43

armanbilge reviewed Nov 16, 2023

View reviewed changes

iRevive force-pushed the sdk-trace/span-exporter branch 3 times, most recently from fafb79c to 06a14f7 Compare November 21, 2023 09:30

armanbilge reviewed Nov 21, 2023

View reviewed changes

iRevive force-pushed the sdk-trace/span-exporter branch from 06a14f7 to bc0411d Compare November 21, 2023 11:30

NthPortal approved these changes Nov 24, 2023

View reviewed changes

armanbilge reviewed Nov 24, 2023

View reviewed changes

iRevive force-pushed the sdk-trace/span-exporter branch 5 times, most recently from 2236f13 to 30a92c6 Compare November 27, 2023 09:02

iRevive force-pushed the sdk-trace/span-exporter branch from 30a92c6 to 2693fdd Compare November 27, 2023 09:15

armanbilge approved these changes Nov 27, 2023

View reviewed changes

sdk-trace: add SpanExporter

642b187

iRevive force-pushed the sdk-trace/span-exporter branch from ad6fd28 to 642b187 Compare November 28, 2023 08:17

iRevive merged commit a1efbec into typelevel:main Nov 28, 2023
10 checks passed

iRevive deleted the sdk-trace/span-exporter branch November 28, 2023 08:44

NthPortal mentioned this pull request Nov 28, 2023

Support using Scala propagators with Java backend #365

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk-trace: add `SpanExporter` #373

sdk-trace: add `SpanExporter` #373

iRevive commented Nov 15, 2023 •

edited

Loading

armanbilge Nov 16, 2023

iRevive Nov 16, 2023

armanbilge Nov 21, 2023

iRevive Nov 21, 2023

armanbilge Nov 21, 2023

armanbilge Nov 21, 2023

iRevive Nov 21, 2023 •

edited

Loading

iRevive Nov 21, 2023 •

edited

Loading

NthPortal left a comment

NthPortal Nov 24, 2023

iRevive Nov 24, 2023

armanbilge Nov 24, 2023 •

edited

Loading

NthPortal Nov 24, 2023

armanbilge Nov 24, 2023

NthPortal Nov 24, 2023

iRevive Nov 24, 2023

NthPortal Nov 24, 2023

iRevive Nov 27, 2023

armanbilge Nov 24, 2023

iRevive Nov 24, 2023

armanbilge Nov 24, 2023

iRevive Nov 24, 2023

armanbilge Nov 25, 2023

iRevive Nov 25, 2023

iRevive Nov 25, 2023

armanbilge Nov 25, 2023

iRevive Nov 25, 2023

iRevive Nov 26, 2023

iRevive commented Nov 27, 2023 •

edited

Loading

iRevive commented Nov 27, 2023

		/** Creates a [[SpanExporter]] which delegates all exports to the exporters in
		* order.

	def exportSpans(span: List[SpanData]): F[Unit]
	def exportSpans(spans: List[SpanData]): F[Unit]

		* depending on the type of span processor being used. However, the batch
		* span exporter will ensure that only one export can occur at a time.

	def exportSpans(spans: List[SpanData]): F[Unit]
	def exportSpans[G[_]: Foldable](spans: G[SpanData]): F[Unit]

sdk-trace: add SpanExporter #373

sdk-trace: add SpanExporter #373

Conversation

iRevive commented Nov 15, 2023 • edited Loading

1) The exporter MUST support three functions: Export, Shutdown, and ForceFlush.

2) export must return the ExportResult

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 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

iRevive Nov 21, 2023 • edited Loading

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

armanbilge Nov 24, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive commented Nov 27, 2023 • edited Loading

iRevive commented Nov 27, 2023

sdk-trace: add `SpanExporter` #373

sdk-trace: add `SpanExporter` #373

iRevive commented Nov 15, 2023 •

edited

Loading

2) `export` must return the `ExportResult`

iRevive Nov 21, 2023 •

edited

Loading

iRevive Nov 21, 2023 •

edited

Loading

armanbilge Nov 24, 2023 •

edited

Loading

iRevive commented Nov 27, 2023 •

edited

Loading