Skip to content

Conversation

@shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Jul 9, 2025

Closes #51

The primary addition here a collector using a Cohttp_eio client to collect signals in programs using Eio. Additional changes include:

  • Some adaptations to the testing harness from Factor batching logic out of the cohttp-lwt client #97, accounting for Cohttp_eios different preferences round networking (defaulting to IPv6).
  • An Eio based emitter executable that is isomorphic to the Lwt-based emit1-cottp.ml.
  • An instantiation of the same signal gathering test suite used for the Lwt collector using the Eio emitter. This lets us confirm that the exact same tests which work to validate the Lwt based collector also check out for the Eio based collector.

Todo

  • Rebase on main once Add integration tests for collectors #96 is merged
  • Fix CI test failure due to needing to accommodate different sets of packages being run on different ocaml versions in the build matrix.
  • Fix hack in Http client configuration, so we can support https

@shonfeder shonfeder changed the title Add eio backend Add Eio collectort Jul 9, 2025
@shonfeder shonfeder changed the title Add Eio collectort Add Eio collector Jul 9, 2025
@shonfeder shonfeder force-pushed the add-eio-backend branch 2 times, most recently from b2aa8ed to 4f1c1ae Compare July 10, 2025 21:11
@shonfeder shonfeder marked this pull request as ready for review July 10, 2025 21:43
@shonfeder shonfeder requested a review from c-cube as a code owner July 10, 2025 21:43
@shonfeder shonfeder marked this pull request as draft July 10, 2025 21:43
@shonfeder shonfeder closed this Jul 10, 2025
@shonfeder shonfeder reopened this Jul 10, 2025
@shonfeder shonfeder force-pushed the add-eio-backend branch 6 times, most recently from f5eae04 to 1787d42 Compare July 11, 2025 18:30
@shonfeder shonfeder marked this pull request as ready for review July 11, 2025 18:42
@shonfeder
Copy link
Contributor Author

This should be ready for review when you have availability!

build:
strategy:
fail-fast: true
fail-fast: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we want to fail fast any more, since we test different packages in different parts of the build matrix.

Copy link
Member

@c-cube c-cube left a comment

Choose a reason for hiding this comment

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

That's a lot! But I think it looks solid. I only have minor qualms, then I'll merge :)

@shonfeder
Copy link
Contributor Author

That's a lot!

Yes, I'm sorry I wasn't able to break this into smaller bits given my time frame. Tho it was smaller than the first draft at least 😅

Thank you for the review and for catching the issues you noticed!

@c-cube
Copy link
Member

c-cube commented Jul 14, 2025

seems like the package is still dependent on eio_main. Should it be just a test-dep?

@shonfeder
Copy link
Contributor Author

Fixed now I think!
🤦

@shonfeder shonfeder closed this Jul 14, 2025
@shonfeder shonfeder reopened this Jul 14, 2025
@c-cube c-cube merged commit 0751313 into imandra-ai:main Jul 14, 2025
11 of 12 checks passed
@c-cube
Copy link
Member

c-cube commented Jul 14, 2025

Thank you! :)

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.

Collector that uses EIO

3 participants