Skip to content

Conversation

@shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Jun 27, 2025

This adds

  • A simple signal gathering server that can be run to collect signals sent by an application instrumented with a collector.
  • Changes to the lwt and ocurl emitters to bring them into parity and support testing
  • An integration test harness for our otel collectors, using this server.
  • Integration tests for the cotthp-lwt collector.

Initial, obviated discussion from when PR was opened as a draft

This relies on a pair of hacks for getting deterministic output of collected signals, which are then used for expect-style testing.

These hacks are sufficient (locally, at least) for the cohttp-lwt emitter, but still yield non-deterministic output for the ocurl emitter. I have in mind two routes for proper testing here:

  1. Abandon the expect-based approach. While it seems convenient at first, it is really not suitable for this domain, because
    • (a) the outputs contain non-deterministic data and
    • (b) outside of the trivial case, there is way too much output for the diffs to be very useful. Instead, we can feed the collected signals into an alcotest suite, and just check properties of the collected data.
  2. Keep to the expect-based approach, despite the reservations of (1), but project the signal data into a flattened form, and apply an ordering to get determinstic output.

I'm opening this as a draft for now, in case anyone is motivated to share opinions on the approach so far, or suggest other ones.

@c-cube
Copy link
Member

c-cube commented Jul 3, 2025

I think 1. could make sense if we have some testing utils to "traverse" a collection of signals. We don't need to encode them either, we can just use the Opentelemetry OCaml values. It's certainly the most robust way to ignore the nondeterministic parts.

@shonfeder
Copy link
Contributor Author

shonfeder commented Jul 3, 2025

Excellent, this makes the most sense to me too. Thanks!

I am porting the testing server to lwt, to address that part of the issue, and I think I have a clear sense of how to approach the other issue. Thanks!

shonfeder added 9 commits July 8, 2025 21:28
We reset the encoder if we are reusing one, and we generate a fresh new
one otherwise.
Don't know why I didn't opt for this clearer name originally.
No reason to keep this value hidden, and we want to reuse it for tests.
These combinators seem tiny, but they simpflify code where they are used
quite a lot.
@shonfeder shonfeder changed the title WIP: Add integration tests for collectors Add integration tests for collectors Jul 9, 2025
@shonfeder shonfeder mentioned this pull request Jul 9, 2025
3 tasks
@shonfeder
Copy link
Contributor Author

Thanks @c-cube! I've made the changes using the approach we agreed was preferable. In #98 you can see how we will use the exact same conformance tests for the WIP Eio collector.

@shonfeder shonfeder marked this pull request as ready for review July 9, 2025 01:48
@shonfeder shonfeder requested a review from c-cube as a code owner July 9, 2025 01:48
@c-cube
Copy link
Member

c-cube commented Jul 10, 2025

I think we could use containers or astring as a test dep, for this kind of issue we see in CI. Or we can bump the minimal OCaml version to 4.13 at least.

Also document the signal reporter executable
To get access to useful functions that are not in the Stdlib in OCaml 4.08.
@shonfeder
Copy link
Contributor Author

Yeah, that is nicer than copy pasta of these functions into the tests code :D .

I've opted for Containers, and done a little cleanup pass, and CI is green!

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.

Excellent, thank you!

@shonfeder
Copy link
Contributor Author

Thank you for the review!

@c-cube c-cube merged commit a5af3c9 into imandra-ai:main Jul 12, 2025
4 checks passed
@shonfeder shonfeder deleted the tests branch July 12, 2025 16:33
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