Skip to content
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

sdk-trace: add SimpleSpanProcessor #388

Merged
merged 1 commit into from
Dec 16, 2023

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Nov 28, 2023

Reference Link
Spec https://opentelemetry.io/docs/specs/otel/trace/sdk/#simple-processor
Java implementation SimpleSpanProcessor.java

I also moved SpanProcessor to the org.typelevel.otel4s.sdk.trace.processor package.

@iRevive iRevive added tracing Improvements to tracing module sdk module Features and improvements to the sdk module labels Nov 28, 2023
@iRevive iRevive force-pushed the sdk-trace/simple-span-processor branch from 459c456 to 9595a0f Compare November 28, 2023 10:26

private def doExport(span: SpanData): F[Unit] =
exporter.exportSpans(List(span)).onError {
case e if NonFatal(e) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You never need non-fatal checks within the effect, if a non-fatal error has been countered the JVM has already terminated 😁

Also onError just-side-channels, so the error continues being raised. Wouldn't this possibly lead to duplicate error reporting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Let's not propagate errors, at least for now. We can reconsider the choice once we have the fully working SDK in place.

Comment on lines 63 to 64
_ <- Console[F].errorln("SimpleSpanExporter: the export has failed")
_ <- Console[F].printStackTrace(e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One unfortunate thing about logging in two steps it that it may be come separated if there are other interleaving logs. It might be better to make a new exception that is caused by e and log that, or assemble the string + stacktrace first and then log it atomically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, does it make sense to introduce a separate interface for logging? For example:

trait Log[F[_]] {
  def info(message: => String): F[Unit]
  def error(message: => String): F[Unit]
  def error(message: => String, error: => Throwable): F[Unit]
}

We will use the cats.effect.std.Console by default. But a user can plug in any other logging backend: log4cats, odin, scribe, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all starts to feel rather circular 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree. Console should be more than enough for our use cases (at least now 😏 ).

@iRevive iRevive force-pushed the sdk-trace/simple-span-processor branch from 2201366 to cb534e6 Compare December 16, 2023 10:23
@iRevive iRevive merged commit 86bdb67 into typelevel:main Dec 16, 2023
10 checks passed
@iRevive iRevive deleted the sdk-trace/simple-span-processor branch December 16, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sdk module Features and improvements to the sdk module tracing Improvements to tracing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants