Make Trace a dynamically dispatched effect #14
Conversation
arybczak
force-pushed
the
dynamic-dispatch
branch
from
68c243c
to
aadb294
Compare
After some thinking I decided that `Trace` should be dynamic since currently it has two interpretations crammed into it based on whether the `Scope` is there or not (another design flaw of the original library). I also renamed `Tracing` to `Trace` to keep naming consistent with TraceT and MonadTrace. I also don't think `addSpanEntry` should do anything in the no-op handler.
arybczak
force-pushed
the
dynamic-dispatch
branch
from
aadb294
to
69d9f98
Compare
There was a problem hiding this comment.
LGTM. It's interesting to see a dynamic dispatch implementation.
Just an aside: you changed the order of parameters in runTrace (formerly runTracing) and runZipkinTracing; though I understand that the new order makes more sense, I had kept the order of the equivalent MonadTrace functions mostly because I thought it would avoid some headscratching when switching a project from MonadTrace to Effectful.
| withZipkin :: (IOE :> es) => Settings -> (Zipkin -> Eff es a) -> Eff es a | ||
| withZipkin settings f = do | ||
| zipkin <- unsafeEff_ $ new settings | ||
| zipkin <- liftIO $ new settings |
There was a problem hiding this comment.
For my general understanding of effectful: are liftIO and unsafeEff_ exchangeable in general (or at least when es is mostly IOE ?) or is this a specific case ?
There was a problem hiding this comment.
Not quite - liftIO requires IOE effect to be in scope and unsafeEff_ doesn't - that's why it's called unsafe ;)
It's an escape hatch mostly for statically dispatched effect operations which shouldn't require IOE (only the runner should), but I was unable to find a more satisfying solution to this problem so far.
| f zipkin `finally` publish zipkin | ||
|
|
||
| -- | Runs a 'TraceT' action, sampling spans appropriately. Note that this method does not publish | ||
| -- spans on its own; to do so, either call 'publish' manually or specify a positive | ||
| -- 'settingsPublishPeriod' to publish in the background. | ||
| runZipkinTracing :: (IOE :> es) => Eff (Trace : es) a -> Zipkin -> Eff es a | ||
| runZipkinTracing actn zipkin = runTracing actn (zipkinTracer zipkin) | ||
| runZipkinTracing :: (IOE :> es) => Zipkin -> Eff (Trace : es) a -> Eff es a |
There was a problem hiding this comment.
For naming coherence, it should probably renamed as runZipkinTrace.
arybczak
force-pushed
the
dynamic-dispatch
branch
2 times, most recently
from
a11e7a8
to
f58c2f7
Compare
arybczak
force-pushed
the
dynamic-dispatch
branch
from
f58c2f7
to
ccff536
Compare
withZipkin is Monitor.Tracing.Zipkin.with and runZipkinTrace is somewhat unnecessary.
After some thinking I decided that
Traceshould be dynamic since currently it has two interpretations crammed into it based on whether theScopeis there or not (another design flaw of the original library). I also renamedTracingtoTraceto keep naming consistent with TraceT and MonadTrace.I also don't think
addSpanEntryshould do anything in the no-op handler.EDIT: I'll rename
Effectful.TracingtoEffectful.Tracebefore merging, I didn't do it yet because then GitHub generates a bad diff.