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

oteljava: add OtelJava.noop #543

Merged
merged 1 commit into from
Mar 27, 2024
Merged

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Mar 11, 2024

Closes #542.

@NthPortal
Copy link
Contributor

I'm not sure how I feel about this. I'm not convinced that it's necessary, given that Tracer.Implicits.noop and Meter.Implicits.noop exist

@iRevive
Copy link
Contributor Author

iRevive commented Mar 12, 2024

In some scenarios, OtelJava can be treated as a superior telemetry interface with access to metrics, traces, and local context.

For example, when dealing with big and modular apps:

class StreamingModule[F[_]: OtelJava]
class ScoringModule[F[_]: OtelJava]
class HttpModule[F[_]: OtelJava]

vs:

class StreamingModule[F[_]: MeterProvider: TracerProvider: LocalContext]
class ScoringModule[F[_]: MeterProvider: TracerProvider: LocalContext]
class HttpModule[F[_]: MeterProvider: TracerProvider: LocalContext]

Also, JOpenTelemetry has noop.

@iRevive iRevive added the module:oteljava Features and improvements to the oteljava module label Mar 12, 2024
@fugafree
Copy link

Hi, I'm going to leave my notes here about @NthPortal's question:

My use cases:

  • context propagation: OtelJava#propagators.textMapPropagator: solvable as well with ContextPropagators.noop()
  • get Context via OtelJava#localContext: this is the problematic one
  • get underlying JOpenTelemetry if needed: OtelJava#underlying: solvable like you mentioned

What you just mentioned is how my code looks like at the moment:

for {
  noopOpenTelemetry <- IO(OpenTelemetry.noop())
  otelJava          <- OtelJava.forAsync[IO](noopOpenTelemetry)
} yield ...

My problem with this, is that this is in an effect, and I need to call unsafeRunSync in my tests every time I would like to use a noop instance. Which is not a huge problem to be honest, and maybe it's just not worth that ugly code (the val noopLocal: LocalContext[F]). To be honest, I also tried to implement this on my own and faced with this LocalContext issue as well.

And some side note:
I'm passing around an OtelJava instance, instead of : MeterProvider: TracerProvider: LocalContext just like how @iRevive mentioned. This is one of the reason why this noop would be useful for me.

And to have a bit of a confession here, I'm not passing around an OtelJava, but a custom case class. This is how my code looks like actually:

case class Telemetry(
    openTelemetry: OpenTelemetry,   // until underlying is not exposed
    otel4s: OtelJava[IO],           // here for the above reasons
    meter: Meter[IO],               // because I don't need a provider, I need a meter in my code
    tracer: Tracer[IO]              // because I don't need a provider, I need a tracer in my code
)

Conclusion: if you are not comfortable with the current noop proposal because of that noopLocal I can totally understand that, and I'm okey with the forAsync way. Although packing everything into one single class would be pretty cool.

@iRevive
Copy link
Contributor Author

iRevive commented Mar 12, 2024

My problem with this, is that this is in an effect, and I need to call unsafeRunSync in my tests every time I would like to use a noop instance.

With the proposed implementation of the noop instance, you will still need to run .unsafeRunSync() at least once:

private val noopOtelJava: OtelJava[IO] = {
  implicit val local: Local[IO, Context] = LocalProvider[IO, Context].local.unsafeRunSync()
  OtelJava.noop
}

The closest alternative that is currently available (in 0.5.0-RC1):

private val noopOtelJava: OtelJava[IO] = {
  implicit val local: Local[IO, Context] = LocalProvider[IO, Context].local.unsafeRunSync()
  OtelJava.local(JOpenTelemetry.noop)
}

@NthPortal
Copy link
Contributor

get Context via OtelJava#localContext: this is the problematic one

if you're using a no-op implementation, what do you need context for? there's no span data in it

@fugafree
Copy link

With the proposed implementation of the noop instance

No, if I use a val noopLocal: LocalContext[F]... But again, not so pretty.

if you're using a no-op implementation, what do you need context for? there's no span data in it

My classes expect an OtelJava instance, and in several places I access to the context.
For example

  • interop with Java code where no other solution is available (like for setting context for logs)
  • extract Context to pass it to TextMapPropagator.inject (hmm, why do I need to do this, it would be much more nicer if TextMapPropagator would have a version, which works with the context stored in local)
  • read baggage

When I run my tests, I'd like to pass a noop OtelJava instance for them.

@iRevive
Copy link
Contributor Author

iRevive commented Mar 15, 2024

extract Context to pass it to TextMapPropagator.inject

You can implement a custom TextMapPropagator and configure otel4s to use it:

import io.opentelemetry.context.propagation.{TextMapPropagator => JTextMapPropagator}
import org.typelevel.otel4s.context.propagation.{TextMapGetter, TextMapPropagator, TextMapUpdater}
import org.typelevel.otel4s.oteljava.context.Context
import org.typelevel.otel4s.oteljava.context.propagation.PropagatorConverters._

val customPropagator = new TextMapPropagator[Context] {
  val fields: Iterable[String] = List("custom_header") // that's a utility info
  def extract[A: TextMapGetter](ctx: Context, carrier: A): Context = ???
  def inject[A: TextMapUpdater](ctx: Context, carrier: A): A = ???
}

OtelJava.autoConfigured(
  _.addPropagatorCustomizer((t, c) => JTextMapPropagator.composite(t, customPropagator.asJava) )
)

But if you use the noop implementation, the propagation will not work.

@NthPortal
Copy link
Contributor

NthPortal commented Mar 15, 2024

extract Context to pass it to TextMapPropagator.inject

a no-op OtelJava would very explicitly not do this, as it has a no-op propagator.

(hmm, why do I need to do this, it would be much more nicer if TextMapPropagator would have a version, which works with the context stored in local)

see Tracer#joinOrRoot and Tracer#propagate.

interop with Java code where no other solution is available (like for setting context for logs)

read baggage

I don't fully understand the details of these, but in general your strategy has the issue that you're trying to get an implementation that performs no operations to do things, which is incorrect. I would say if you just want working context and nothing else, wrapping the Java OpenTelemetry.noop() is probably your best bet.

we can even potentially have our wrappers of the Java Tracer and Meter check if they're wrapping the Java no-op implementations and perform the macro optimisations in that case.


please note that the Scala and Java contexts do not automatically interoperate at this time, and you will need to manually transfer the context at boundaries anyway. the eventual solution to this is #214

@NthPortal
Copy link
Contributor

NthPortal commented Mar 15, 2024

IO(OpenTelemetry.noop())

@fugafree I don't think you really need to wrap it in IO, it does't have any side effects (it's just cached/memoised allocation)

@fugafree
Copy link

fugafree commented Mar 16, 2024

a no-op OtelJava would very explicitly not do this, as it has a no-op propagator.

And this is what I want. The propagation code is in my prod code, and sometimes the "backend" telemetry is noop, for tests.

see Tracer#joinOrRoot and Tracer#propagate.

Wow, very nice. Thank you!

interop with Java + read baggage

I meant that there will be always scenarios, when I will need to get a java Context (interop + read baggage from context). Just like the interop example here: https://typelevel.org/otel4s/instrumentation/tracing-java-interop.html#how-to-use-otel4s-context-with-java-sdk
And for that I will need to have some LocalContext.

To summarize: this seems to be the best way of having a noop otel4s after all:

OtelJava.forAsync[F](OpenTelemetry.noop())

By having this discussion, I'm perfectly okey with this.

If you are comfortable with closing this PR and the issue without merging it, feel free to do so.

@iRevive
Copy link
Contributor Author

iRevive commented Mar 18, 2024

Use cases aside, I see the following pattern:

  1. Otel4s interface is an entry point to the telemetry instruments: metrics and traces
  2. OtelJava is a wrapper over JOpenTelemetry, JOpenTelemetry is an entry point to the telemetry instruments too
  3. JOpenTelemetry allows constructing a no-op instance via JOpenTelemetry.noop()
  4. OtelJava also provides a utility method global, which is a basically a combination of OtelJava.forAsync(JGlobalOpenTelemetry.get)

Following this reasoning, OtelJava.noop can be a helpful addition to the API.


The definition, however, can be different.
I see two options:

def noop[F[_]: Applicative: LocalContext]: OtelJava[F]

Unfortunately, a user must provide LocalContext, which may be confusing.

In most cases, a user will end up doing LocalContextProvider[F].local.map(implicit local => OtelJava.noop[F]) (which is option 2).

def noop[F[_]: Applicative: LocalContextProvider]: F[OtelJava[F]]

Well, you need to evaluate an effect to get a noop instance. On the other hand, everything works out of the box.


Another unrelated thought is that you can always use OtelJava.autoConfigured(). If you provide otel.sdk.disabled=true (either via properties or an env variable) you will get a no-op instance.

This works fine when you run an application. However, in tests I would prefer an explicit OtelJava.noop.

@fugafree
Copy link

I would extend this a bit:

Otel4s interface is an entry point to the telemetry instruments: metrics and traces

It is also used to reach internal context, because that is the only tool at the moment to

  • manage baggage
  • interop with java libs

Another thought for

However, in tests I would prefer an explicit OtelJava.noop.

Yes, would be nice, that's why I opened the Issue. Although not very critical.

@NthPortal
Copy link
Contributor

It is also used to reach internal context

this is not quite correct. OtelJava can be used to access the context, but Otel4s does not provide that functionality at this time

Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

I continue to feel dubious about this, but if it serves a need, then let's go for it

@iRevive iRevive merged commit 01e473e into typelevel:main Mar 27, 2024
10 checks passed
@iRevive iRevive deleted the oteljava/noop branch November 29, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:oteljava Features and improvements to the oteljava module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OtelJava.noop()
3 participants