Skip to content

Allow IOG's contra-tracer #766

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

Conversation

jasagredo
Copy link

Description

A first attempt at integrating this into ouroboros-consensus showed a misalign in the contra-tracer version. Changes were sufficiently simple that I think it is reasonable to put some CPPs here and there and be done while someone else figures out how to reconcile our stack with the Hackage contra-tracer.

Checklist

  • Read our contribution guidelines at CONTRIBUTING.md, and make sure that this PR complies with the guidelines.

lsm-tree.cabal Outdated
Comment on lines 612 to 616
if flag(iog-contra-tracer)
build-depends: contra-tracer ^>=0.1

else
build-depends: contra-tracer ^>=0.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not yet clear to me why we need the cabal flag. The core library's code works with 0.2 from Hackage, 0.1 from Hackage, and 0.1 from CHaP, right? What would happen if we just put contra-tracer ^>=0.1 || ^>=0.2 here, without the cabal flag?

Copy link
Author

Choose a reason for hiding this comment

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

It might work, indeed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My suggestion would be to do the following, which I tried locally and seemed to work:

diff --git a/lsm-tree.cabal b/lsm-tree.cabal
index 6ba33cfd..579b6cc8 100644
--- a/lsm-tree.cabal
+++ b/lsm-tree.cabal
@@ -582,7 +582,7 @@ library
     , bytestring              ^>=0.11.4.0 || ^>=0.12.1.0
     , cborg                   ^>=0.2.10.0
     , containers              ^>=0.6      || ^>=0.7
-    , contra-tracer           ^>=0.2
+    , contra-tracer           ^>=0.1      || ^>=0.2
     , crc32c                  ^>=0.2.1
     , deepseq                 ^>=1.4      || ^>=1.5
     , filepath
diff --git a/src/Database/LSMTree/Internal/Unsafe.hs b/src/Database/LSMTree/Internal/Unsafe.hs
index 4fd75489..4ad7b644 100644
--- a/src/Database/LSMTree/Internal/Unsafe.hs
+++ b/src/Database/LSMTree/Internal/Unsafe.hs
@@ -202,12 +202,15 @@ data TableTrace =
   | TraceSupplyUnionCredits UnionCredits
   deriving stock Show

-contramapTraceMerge :: Monad m => Tracer m TableTrace -> Tracer m (AtLevel MergeTrace)
+contramapTraceMerge :: forall m. Monad m => Tracer m TableTrace -> Tracer m (AtLevel MergeTrace)
 #ifdef DEBUG_TRACES
 contramapTraceMerge t = TraceMerge `contramap` t
 #else
-contramapTraceMerge t = traceMaybe (const Nothing) t
+contramapTraceMerge _t = nullTracer
 #endif
+  where
+    -- See #766
+    _unused = pure @m ()

 data CursorTrace =
     TraceCreateCursor TableId

Copy link
Collaborator

Choose a reason for hiding this comment

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

The _unused is there purely to fulfill the Monad m constraint, which is obsolete for nullTracer, but it's nicer if we have the same signature regardless of the DEBUG_TRACES flag

Copy link
Author

Choose a reason for hiding this comment

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

I think this looks good. I will push this diff on top of my branch tomorrow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there may have been some misunderstanding, I meant: make only the changes in the diff above, and remove all the other changes that were already there

That would make the core library compatible with the IOG contra-tracer. The tests and benchmarks don't have to change, since we don't run them with the IOG contra-tracer anyway

@jasagredo jasagredo force-pushed the js/iog-contra-tracer branch from fa0d862 to 55c4b99 Compare July 1, 2025 13:43
@jasagredo jasagredo changed the base branch from main to jdral/serialisekey-void July 1, 2025 13:43
@jasagredo jasagredo requested a review from jorisdral July 1, 2025 13:43
@jorisdral jorisdral deleted the branch IntersectMBO:jdral/serialisekey-void July 1, 2025 14:08
@jorisdral jorisdral closed this Jul 1, 2025
@jorisdral
Copy link
Collaborator

Huh, apparently the PR was closed because the base branch disappeared. Could you reopen it @jasagredo ?

The PR history says I closed it but I did no such thing manually 😛

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.

2 participants