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

[Do not merge] Trace API playground #35

Closed
wants to merge 6 commits into from
Closed

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Jul 24, 2022

This branch contains various experiments with OpenTelemetry tracing.

To simply the review process I made two different PRs:

  1. Define API Define tracing API #36
  2. Implementation Implement tracing API #37

Currently, the state is stored in IOLocal. This approach has several drawbacks:

The checklist from #11

  • Provide access to spanId and traceId from Trace[F]
  • Allow creating a new root span from Trace[F]
  • Allow running an effect without tracing
  • Proper tracing of a Resource
  • Ability to trace fs2.Stream
  • Memory pressure when Trace is not used
  • Richer API

@iRevive iRevive changed the title Draft Trace API WIP: Draft Trace API Jul 24, 2022
@iRevive iRevive requested a review from rossabaker July 24, 2022 09:30
@iRevive iRevive force-pushed the tracing branch 2 times, most recently from 0d258a2 to 0171d0a Compare July 28, 2022 09:19
@iRevive
Copy link
Contributor Author

iRevive commented Jul 29, 2022

Examples:

examples/src/main/scala/com/example/tracing/JaegerTracing.scala

Resource span
tracer
  .resourceSpan("resource-span")(makeImageLookup(tracer))
  .use { case Span.Res(lookup) =>
    lookup.exists("my-image")
  }
image
Stream span
Stream
  .resource(tracer.resourceSpan("stream-span")(Resource.unit))
  .flatMap(_ => Stream.emits(Seq(1, 2, 3)))
  .evalMap(r => tracer.span(s"span-$r").surround(IO.unit))
  .compile
  .drain
image

Stream span is not ideal, but at least it works out of the box and does not require the fs2 dependency.

@iRevive
Copy link
Contributor Author

iRevive commented Jul 29, 2022

The change set is quite big and it's hard to review it.
I can try to split it into smaller pieces. Two different PRs come to mind:

  1. Solely API definitions
  2. Implementation and tests

Comment on lines +34 to +38
def traceId: F[Option[String]]

/** Returns span identifier of a span that is available in a scope.
*/
def spanId: F[Option[String]]

Choose a reason for hiding this comment

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

I realise this is WIP so not sure if you plan to change this or if the Otel spec has changed, but these are usually represented as byte arrays; e.g TraceId and SpanId in t4c. It would be good to be able to access these directly rather than just the string representation.

Apologies if I've jumped the gun and you're going to change these, or I've missed something and they're represented elsewhere!

Copy link
Contributor Author

@iRevive iRevive Jul 29, 2022

Choose a reason for hiding this comment

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

It's a draft version and my gut feeling says there will be a lot of changes :)
It would be interesting for me to hear your opinion about the current API.

Open Telemetry has the following info in span context:

String getTraceId();
byte[] getTraceIdBytes();
String getSpanId();
byte[] getSpanIdBytes();
boolean isSampled();
TraceFlags getTraceFlags();
TraceState getTraceState();
boolean isValid();
boolean isRemote();

I believe we can incorporate the majority of attributes.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be happy to see scodec-bits ByteVector used for byte arrays :) it's a stable dependency used in fs2-core and beyond.

Choose a reason for hiding this comment

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

ah yeah, that makes sense. What we do in t4c is have Show instances for TraceId which makes those a bit more FP'y.

Also I made the the isSampled return an ADT that can turn into a boolean, purely because everyone (including myself) kept getting confused as to what true meant when it came to sampling!

In terms of the trace context in t4c it more closely follows the W3C standard, so in terms of structure probably doesn't align very well with the Otel spec.

}
} yield new SpanBackendImpl(jTracer, jSpan, scope)

def reportStatus(backend: Span.Backend[F], ec: Resource.ExitCase): F[Unit] =
Copy link
Contributor Author

Choose a reason for hiding this comment

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


def acquire: F[SpanBackendImpl[F]] =
for {
now <- Sync[F].realTime
Copy link
Contributor Author

Choose a reason for hiding this comment

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

OpenTelemetry requires it to be a realTime rather than monotonic

@iRevive iRevive marked this pull request as draft July 30, 2022 13:55
@iRevive iRevive changed the title WIP: Draft Trace API Draft Trace API Jul 30, 2022
@iRevive iRevive mentioned this pull request Jul 30, 2022
@iRevive iRevive changed the title Draft Trace API Draft: Trace API playground Jul 30, 2022
@iRevive iRevive changed the title Draft: Trace API playground [Do not merge] Trace API playground Jul 30, 2022
@iRevive
Copy link
Contributor Author

iRevive commented Aug 4, 2022

I'm closing this PR since it's not relevant anymore.

@iRevive iRevive closed this Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants