Add tracing-effectful package #11

Raveline · 2024-03-27T15:21:33Z

This is a proposed solution to implement CORE-6093.

This has been tested on a dummy client like this:

{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE TypeOperators #-}
module Main where

import Prelude hiding (putStrLn)
import Effectful ((:>), Eff, runEff)
import Effectful.Concurrent (threadDelay, Concurrent, runConcurrent)
import Effectful.Console.ByteString (putStrLn, Console, runConsole)
import Effectful.Tracing (Trace, runZipkinTracing, withZipkin)
import Control.Monad (forever)
import Control.Monad.Base (MonadBase, liftBase)
import Monitor.Tracing
import Control.Monad.Trace.Class (rootSpanWith)
import qualified Monitor.Tracing.Zipkin as ZPK 

main :: IO ()
main = runEff . runConsole . runConcurrent . withZipkin settings $ runZipkinTracing runner

settings :: ZPK.Settings
settings = 
  ZPK.defaultSettings
    { ZPK.settingsEndpoint =
      Just $
        ZPK.defaultEndpoint
          { ZPK.endpointService = Just "test"
          }
          , ZPK.settingsPublishPeriod = 1 -- second
    }   

runner :: (Trace :> es, Concurrent :> es, Console :> es) => Eff es ()
runner = rootSpanWith (ZPK.addInheritedTag "id" "1234") alwaysSampled "example" $
  forever $ do
    childSpan "wakeup" wakeup
    threadDelay 5000000

wakeup :: (Trace :> es, Concurrent :> es, Console :> es) => Eff es ()
wakeup = do
  putStrLn $ "Woke up"
  threadDelay 3000000
  childSpan "subcall" doubleWakeup

doubleWakeup :: (Trace :> es, Concurrent :> es, Console :> es) => Eff es ()
doubleWakeup = putStrLn $ "Double wake up"

I'd reaaaaaally wish to find a less ugly solution to the implementation of traceWith.

I'll detail a bit the issue. We want to factorize code between the MonadTrace historic implementation and the Trace effect, which is why I put it in a separate, callable function. It has to be a higher-order-function, so that our MonadTrace can give the local function from its MonadReader, and so that our Trace effect can use the equivalent localStaticRep.

However, we can't have the whole traceWith function operate in MonadBaseControl IO m, MonadIO m world, because if we do, the whole call has to be wrapped in unsafeEff_... and then we can't use localStaticRep because we're not in Eff anymore.

I tried many things, and the last one I could find was to use MonadMask: since Eff has an instance, we can stay in Eff in traceWith. We "just" have to provide a lifting function: liftBase in our MonadTrace and the infamous unsafeEff_ in our Trace effect. There might be a better way to do it (there most likely is), but I'm not smart enough (or hopefully, not experienced enough with Effectful) to find it.

(Also, splitting the traceWith function in two part, to get the part that actually produces the spn on the one hand and the conditional branch on the other, would perhaps be a good improvement, but it's outside the scope of this PR).

arybczak · 2024-04-02T15:41:51Z

Thanks 👍 I pushed a commit with a PoC that monomorphizes traceWith to work in IO. Don't worry, these unlifts can be a bit tricky if you're not familar with them.

Can you clean it up? :)

Raveline · 2024-04-02T15:44:02Z

Oooh, that's very cool. Will gladly integrate and cleanup, thanks !

arybczak · 2024-04-02T15:45:35Z

tracing-effectful/src/Effectful/Tracing.hs

+  -> Eff es a
+runTracing actn = runTracing'  actn . pure
+
+runTracing' 


Suggested change

runTracing'

runTracingMaybe

Do you remember our conversation about functions with ticks at the end? ;)

I did, but I was just mimicking what was done in Control.Monad.Trace !

arybczak · 2024-04-02T15:46:21Z

tracing-effectful/tracing-effectful.cabal

@@ -0,0 +1,114 @@
+cabal-version:      3.0
+-- The cabal-version field refers to the version of the .cabal specification,


You can remove these cabal comments, they're quite noisy.

arybczak · 2024-04-02T15:49:49Z

tracing-effectful/tracing-effectful.cabal

+    -- other-extensions:
+
+    -- Other library packages from which modules are imported.
+    build-depends:   base ^>=4.17.2.1


This makes it work with GHC 9.4 only. Also, please put upper bound for base to be 5 so we don't have to bump it when updating GHC.

arybczak · 2024-04-02T15:50:42Z

tracing-effectful/tracing-effectful.cabal

+
+-- Extra doc files to be distributed with the package, such as a CHANGELOG or a README.
+extra-doc-files:    CHANGELOG.md
+


Please add the tested-with field (as is in tracing.cabal) and regenerate CI with haskell-ci (haskell-ci regenerate in the repo root will do the job).

Well, that "Done" was a bit optimistic ! Took me quite a while to get the CI to build everything - oddly enough, some of the build plans seemed to be different between the CI and my own machine, though I switched to the same GHC and cabal version as the ones that failed on the CI. But this is now fixed.

arybczak · 2024-04-03T19:54:08Z

src/Control/Monad/Trace.hs

-instance (MonadBaseControl IO m, MonadIO m) => MonadTrace (TraceT m) where
+traceWith :: Builder -> Scope -> (Scope -> IO a) -> IO a
+traceWith bldr parentScope f = do
+  let mbParentSpn = scopeSpan parentScope


Aren't all these liftIO calls redundant?

Of course they are 🤦‍♂️ Fixed, as well as the liftBases in addSpanEntryWith.

arybczak · 2024-04-03T19:54:23Z

src/Control/Monad/Trace.hs

+  let mbTV = scopes >>= scopeTags
+  for_ mbTV $ \tv -> liftBase . atomically . modifyTVar' tv $ Map.insert key val
+addSpanEntryWith scopes key (LogValue val mbTime) = do
+  let mbTV = scopes >>= scopeLogs


Same as above, but liftBase.

Also apply cabal-fmt to our cabal files while we're at it

arybczak

Not sure why you merged this since I wasn't finished with the review.

arybczak · 2024-04-04T10:06:09Z

tracing-effectful/cabal.project

@@ -0,0 +1,3 @@
+packages:
+  .
+  ../


This is redundant, the cabal project file should be in the root only 🤔

Without this, tracing-effectful tries to fetch the hackage version of tracking.

arybczak · 2024-04-04T10:06:46Z

tracing.cabal

-
-tested-with: GHC == { 8.10.7, 9.0.2, 9.2.8, 9.4.8, 9.6.4, 9.8.2 }
+tested-with:
+  GHC ==8.10.7 || ==9.0.2 || ==9.2.8 || ==9.4.8 || ==9.6.4 || ==9.8.2


Why was this changed? Did you use cabal-fmt or something?

Yes, I used cabal-fmt. Do you prefer the { } version ?

arybczak · 2024-04-04T10:07:15Z

tracing.cabal

+    , aeson              <=2.3
+    , base               <5
+    , base16-bytestring  <=1.1
+    , bytestring         <=0.13


I'm not sure why you added all these upper bounds here.

Upper bounds should be done with < on major versions, which is really not the case here. Also, most of the times you can get away without adding any because they tend to be extremely annoying to update when bumping deps en masse.

1°) I added them because the CI steps generated by haskell-ci requested them. I had originally blindly followed the CI, which suggested using cabal gen-bounds, but of course this broke when testing against various GHC versions.
2°) I agreed they are extremely annoying, because I had to spend quite a while fixing them. I can remove the bound check on the CI if need be (and remove bounds in the cabal file right after). Would this be a good solution ?

Raveline · 2024-04-04T11:31:24Z

Not sure why you merged this since I wasn't finished with the review.

I thought the PR was approved, I completely forgot there is no "approval required" before merging on this repo.

[CORE-6093] Add abstract implementation for MonadTrace

2ddd645

Raveline force-pushed the dev-raveline-core-6093-add-effectful-package branch 2 times, most recently from 3d98bc1 to def458b Compare April 2, 2024 10:53

[CORE-6093] Add effectful package

0e505ba

Raveline force-pushed the dev-raveline-core-6093-add-effectful-package branch from def458b to 0e505ba Compare April 2, 2024 12:32

Raveline marked this pull request as ready for review April 2, 2024 12:35

Raveline requested a review from arybczak April 2, 2024 12:35

PoC for monomorphic traceWith and addSpanEntryWith

6a282d7

arybczak reviewed Apr 2, 2024

View reviewed changes

Raveline force-pushed the dev-raveline-core-6093-add-effectful-package branch 2 times, most recently from 48ab66a to 82a2e34 Compare April 3, 2024 15:27

[CORE-6093] Clean up implementation and cabal file

4da4c86

Raveline force-pushed the dev-raveline-core-6093-add-effectful-package branch from 82a2e34 to 2ee6557 Compare April 3, 2024 15:34

arybczak reviewed Apr 3, 2024

View reviewed changes

Raveline added 2 commits April 4, 2024 09:20

[CORE-6093] Use monomorphized version of traceWith and addSpanEntryWith

b91f88f

[CORE-6093] Add CI to additional package

53b8d49

Raveline force-pushed the dev-raveline-core-6093-add-effectful-package branch 10 times, most recently from ed9f65b to 4c35bd6 Compare April 4, 2024 08:37

[CORE-6093] Satisfy CI by adding bounds

98c2710

Also apply cabal-fmt to our cabal files while we're at it

Raveline force-pushed the dev-raveline-core-6093-add-effectful-package branch from 4c35bd6 to 98c2710 Compare April 4, 2024 08:41

Raveline merged commit 78ce72a into master Apr 4, 2024
6 checks passed

Raveline deleted the dev-raveline-core-6093-add-effectful-package branch April 4, 2024 08:48

arybczak reviewed Apr 4, 2024

View reviewed changes

Raveline mentioned this pull request Apr 4, 2024

Fix unneeded bounds in cabal files and syntax of tested-with #13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tracing-effectful package #11

Add tracing-effectful package #11

Raveline commented Mar 27, 2024 •

edited

Loading

arybczak commented Apr 2, 2024

Raveline commented Apr 2, 2024

arybczak Apr 2, 2024

Raveline Apr 3, 2024

arybczak Apr 2, 2024

arybczak Apr 2, 2024 •

edited

Loading

arybczak Apr 2, 2024

Raveline Apr 3, 2024

Raveline Apr 4, 2024

arybczak Apr 3, 2024

Raveline Apr 4, 2024

arybczak Apr 3, 2024

arybczak left a comment

arybczak Apr 4, 2024

Raveline Apr 4, 2024

arybczak Apr 4, 2024

Raveline Apr 4, 2024

arybczak Apr 4, 2024

Raveline Apr 4, 2024

Raveline commented Apr 4, 2024

		@@ -0,0 +1,114 @@
		cabal-version: 3.0
		-- The cabal-version field refers to the version of the .cabal specification,


		-- Extra doc files to be distributed with the package, such as a CHANGELOG or a README.
		extra-doc-files: CHANGELOG.md

Add tracing-effectful package #11

Add tracing-effectful package #11

Conversation

Raveline commented Mar 27, 2024 • edited Loading

arybczak commented Apr 2, 2024

Raveline commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arybczak Apr 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arybczak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raveline commented Apr 4, 2024

Raveline commented Mar 27, 2024 •

edited

Loading

arybczak Apr 2, 2024 •

edited

Loading