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

Utility for creating an no-op Tracer #206

Open
chris-martin opened this issue Oct 18, 2024 · 13 comments
Open

Utility for creating an no-op Tracer #206

chris-martin opened this issue Oct 18, 2024 · 13 comments

Comments

@chris-martin
Copy link
Contributor

I just had the annoyance of adapting an application that doesn't use tracing to a library that does; I need a no-op Tracer to satisfy a MonadTracer constraint.

It would be neat if MonadTracer were actually something like getTracer :: m (Maybe Tracer) rather than m Tracer.

Alternatively it would be nice to just have a utility for easily creating a do-nothing Tracer.

noTracer = do
  tracerProvider <- createTracerProvider [] emptyTracerProviderOptions
  pure $ makeTracer tracerProvider "" tracerOptions
@chris-martin chris-martin changed the title Utility for creating an empty Tracer Utility for creating an no-op Tracer Oct 18, 2024
@pbrisbin
Copy link
Member

Personally, I don't find it cumbersome to set up tracing normally even if not needed (yet). You can use OTEL_TRACES_EXPORTER=none (or some day OTEL_SDK_DISABLED) to keep it off at runtime.

Concretely having noTracer would mean:

 loadApp f = do
   -- ...
-  withTracerProvider $ tracerProvider -> do
-    let appTracer = makeTracer tracerProvider "my-app" tracerOptions
-    f App {..}
+  appTracer <- noTracer
+  f App {..}

?

It doesn't seem like a huge win to me, but I guess I'm not against it existing so 👍


That said, I think I'm smelling a bit of a design flaw here. Before saying more, what are the specifics of where you hit this? Which functions require MonadTracer are you trying to use, and what's the nature of the app?

@chris-martin
Copy link
Contributor Author

It's demo-district-manager, and the MonadTracer functions are various database and curriculum fetches. I don't actually have any idea what that app is for or whether it should have tracing, but it doesn't currently have a tracer set up.

@pbrisbin
Copy link
Member

It's demo-district-manager ... I don't actually have any idea what that app is for or whether it should have tracing

It's a service that handles requests for "Demo Districts" -- instances of Freckle with "demo" data used for sales. I think tracing would be great there.

various database

Ideally these would use MonadSqlBackend or MonadSqlTx and not force you to satisfy MonadTracer, right?

curriculum fetches

Don't these use MonadHttp, and also not force MonadTracer on you (unless you decide to write your instance to trace)?

@chris-martin
Copy link
Contributor Author

I think tracing would be great there.

Alright but what if I just don't want to do that right now

Don't these use MonadHttp

The tracing that is common to all HTTP requests is hidden under the MonadHttp constraint within the instance, yes. But that's irrelevant; a utility that does multiple fetches or some nontrivial computation can easily want to call inSpan for other reasons, and thus have a MonadTracer constraint.

@chris-martin
Copy link
Contributor Author

It's exactly the same use case as NoLoggingT. Sometimes a tool you're using outputs extra information (logging, tracing, same thing), sometimes you want it, sometimes you don't.

@pbrisbin
Copy link
Member

what if I just don't want to do that right now

Today? Still set up tracing in the code (it's like 2 lines) and use OTEL_TRACE_EXPORTERS=none at runtime.

a utility that does multiple fetches or some nontrivial computation can easily want to call inSpan for other reasons, and thus have a MonadTracer constraint

"a utility" here is (a function in) demo-district-manager? I.e. you're saying it's a valid use-case to author your own thing that calls inSpan, incurs MonadTracer, but then ultimately use noTracer?

@pbrisbin
Copy link
Member

pbrisbin commented Oct 21, 2024

hidden under the MonadHttp constraint within the (AppT?) instance

I think this is the incorrect design I'm smelling. It's that AppT needs MonadTracer to have MonadHttp, right? I think that is what is forcing this thing you don't want/need on you and that is what could use some thought.

@pbrisbin
Copy link
Member

(To reiterate: if you want to implement noTracer, go for it. I'm just following a contrarian lead here in case there's something interesting to uncover.)

Maybe what I'm feeling is this: any given app should decide if its one of two styles.

  1. I want an easy, batteries-included thing, with all out-of-the-box features already there
  2. I want to keep it simple and not bring in unnecessary things

(1) should use freckle-app and AppT, and incur all the constraints that comes with. This means you might have to implement the real HasTracer even if you disable it at runtime, because AppT's MonadHttp requires it.

(2) should use freckle-this and freckle-that, define its own AppT that only has the instances and constraints that are needed. Those instances should be trivial (e.g. DerivingVia), but do need to be written. This makes it possible to not write them.

A function like noTracer gives me pause because it isn't valuable to either camp. And if we implement things that cater to an in-between space like that, then our "opinionated" way to write apps gets muddy, such in-between-space apps perpetuate, we write more stuff to support them, and it continues.

Back to the concrete example: demo-district-manager should probably just be (2), right? It's only set up as (1) because (2) was not possible originally. We might also have not done everything we need to do to best support (2)-style apps. But I'd rather see our efforts spent there. That's partially why I think clearly articulating (1) vs (2) as our use-cases is valuable to get us all rowing in that direction.

🤷

@chris-martin
Copy link
Contributor Author

chris-martin commented Oct 22, 2024

and use OTEL_TRACE_EXPORTERS=none at runtime.

What is the runtime, though? I have no idea what this application is for or where it's run. Since this thing hits the production database, it is theoretically just as capable of causing an incident as anything else, so tracing on it would be good.

"a utility" here is (a function in) demo-district-manager? I.e. you're saying it's a valid use-case to author your own thing that calls inSpan, incurs MonadTracer, but then ultimately use noTracer?

No, it's calling utilities in various other packages in backend that are not simply fetches from other services. There are 20 sites and growing where we add ad hoc spans to poorly performing things that we specifically want to get more runtime visibility on. I think it's reasonable to expect MonadTracer constraints to end up scattered around anywhere and everywhere just like MonadLogger.

MonadHttp

Truly has nothing to do with it

Those instances should be trivial (e.g. DerivingVia), but do need to be written. This makes it possible to not write them.

Yes, my preferred approach here would have been to define a newtype to something like deriving MonadTracer via NoTracer MyAppM. Unfortunately the API of Tracer and MonadTracer makes this awkward, because

  1. Creating a Tracer requires a TracerProvider
  2. A TracerProvider requires IO to acquire

So although I can define noTracer as above (an IO action that creates a no-op tracer), my NoTracer's getTracer would have to re-create the no-op tracer every time it is requested.

instance MonadIO m => MonadTracer (NoTracer m) where
  getTracer = liftIO noTracer

Is this a problem? I'm not really sure, I don't know what the I/O that constructs the tracer provider is or whether it's too expensive to repeat every time you do a tracing operation.

Alternatively... what do you think about just shoving it into an unsafePerformIO global?

noTracer :: Tracer
noTracer = unsafePerformIO $ do
  tracerProvider <- createTracerProvider [] emptyTracerProviderOptions
  pure $ makeTracer tracerProvider "" tracerOptions

newtype NoTracer m a = NoTracer (m a)
  deriving newtype (Functor, Applicative, Monad)

instance Monad m => MonadTracer (NoTracer m) where
  getTracer = pure noTracer

@chris-martin
Copy link
Contributor Author

Anyway I put in a task to add the telemetry so hopefully that thought doesn't get lost.

@pbrisbin
Copy link
Member

so tracing on it would be good

Yup, I think we've already agreed there.

No, it's calling utilities in various other packages in backend

Ah right. The app is not demo-district-manager, it's backend and backend traces. This is why I look at breaking up packages within backend as a bit of a shell game and not actually valuable for decomposition.

[MonadHttp t]ruly has nothing to do with it

Apologies. I thought,

instance (MonadUnliftIO m, HasTracer app) => MonadHttp (AppT app m) where

Was at least part of your issue. And you said some things that I mistook as confirmation. Nevermind.

what do you think about just shoving it into an unsafePerformIO global?

I don't see how it's necessary. Why doesn't the noTracer :: IO Tracer you suggested originally work?

loadApp f = do
  -- ...
  appTracer <- noTracer
  f App {..}

@chris-martin
Copy link
Contributor Author

Why doesn't ... work?

It does, and that's what I'm currently doing, but I thought we wanted to arrive at a solution that could be used only with deriving-via and didn't require adding the appTracer field and extra app initialization step.

@pbrisbin
Copy link
Member

Oh sorry, no that's not what I meant by

Those instances should be trivial (e.g. DerivingVia), but do need to be written. This makes it possible to not write them

I'm not sure what exactly I meant there, but whatever it was, it was a solution to a different problem anyway.

What I thought the problem was: I want to use Freckle.App.AppT app without having to define HasTracer app. This feels like a use-case the library could consider solving. It also feels like a broader class of problem -- that AppT brings a lot of baggage, and avoiding it requires completely re-inventing an AppT yourself -- that I'm constantly bumping into.

Your actual problem: I want to use a bunch of stuff in my own app (backend/) that I've defined to need MonadTracer, but I don't want to define HasTracer app. This doesn't feel like a use-case the library should be too concerned with, and rather seems like something you should solve yourself over in backend/. Using deriving-via and unsafePerformIO would not bother me there, for example.

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

No branches or pull requests

2 participants