Skip to content

Conversation

@shonfeder
Copy link
Contributor

@shonfeder shonfeder commented Aug 1, 2025

The initial implementation of the Eio collector I introduced in #98 was not usable with signals emitted across domains. This was due to the fact that Eio.Switch.t is not thread-safe, and cannot be used across domains, but the implementation passed a switch into the Collector.Backend, which is then accessed across domains:

let create_backend ~sw ?(stop = Atomic.make false) ?(config = Config.make ())
(env : Eio_unix.Stdenv.base) : (module OT.Collector.BACKEND) =
let module E = (val mk_emitter ~sw ~stop ~config ~net:env#net ()) in
(module Backend (E))

Consequently, the core fix introduced here is just to avoid sending a switch created from one domain into a reference that can be accessed from other domains.

However, it took me a bit of careful thinking thru the code flow to fully isolate this problem, and along the way I was able to yield a cleaned implementation.

I also made changes to the integration tests of two sorts:

  1. Introduced a test cases that runs the Eio collector across multiple domains, to regressions.
  2. Changed emit1_eio to produce deterministic signals even when run across multiple domains.

@shonfeder shonfeder force-pushed the eio-fixes branch 4 times, most recently from ee1ce7e to f355287 Compare August 1, 2025 17:37
The backend cannot take a switch, because switches cannot be shared
across domains, but the backend is accessed across domains from a global
variable.
As soon as we start running this in multible system threads, the race to
trigger the globals `stop` and `iterations` makes the signal emissions
non-deterministic, which makes the test kind of meaningless. This change
should make them determinstic.
@shonfeder shonfeder marked this pull request as ready for review August 1, 2025 18:22
@shonfeder shonfeder requested a review from c-cube as a code owner August 1, 2025 18:22
@shonfeder
Copy link
Contributor Author

@shonfeder
Copy link
Contributor Author

Gentle ping here, just to keep in contending in the queue :D

semgrep-ci bot pushed a commit to semgrep/semgrep that referenced this pull request Sep 1, 2025
Lets pin to the latest PR fixing otel eio: imandra-ai/ocaml-opentelemetry#103

I also updated the makefile so semgrep.opam has semgrep.opam.template as a build dependency.

Test plan:
CI Passes

synced from Pro 42be25ccf05a04caaede63f3c87e2c3468864185
semgrep-ci bot pushed a commit to semgrep/semgrep that referenced this pull request Sep 2, 2025
Lets pin to the latest PR fixing otel eio: imandra-ai/ocaml-opentelemetry#103

I also updated the makefile so semgrep.opam has semgrep.opam.template as a build dependency.

Test plan:
CI Passes

synced from Pro 42be25ccf05a04caaede63f3c87e2c3468864185
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. Sorry it took me a while to review it.

@c-cube c-cube merged commit d9dd7ce into imandra-ai:main Sep 2, 2025
7 of 8 checks passed
semgrep-ci bot pushed a commit to semgrep/semgrep that referenced this pull request Sep 2, 2025
Lets pin to the latest PR fixing otel eio: imandra-ai/ocaml-opentelemetry#103

I also updated the makefile so semgrep.opam has semgrep.opam.template as a build dependency.

Test plan:
CI Passes

synced from Pro 42be25ccf05a04caaede63f3c87e2c3468864185
semgrep-ci bot pushed a commit to semgrep/semgrep that referenced this pull request Sep 3, 2025
Lets pin to the latest PR fixing otel eio: imandra-ai/ocaml-opentelemetry#103

I also updated the makefile so semgrep.opam has semgrep.opam.template as a build dependency.

Test plan:
CI Passes

synced from Pro 42be25ccf05a04caaede63f3c87e2c3468864185
nmote pushed a commit to semgrep/semgrep that referenced this pull request Sep 3, 2025
Lets pin to the latest PR fixing otel eio: imandra-ai/ocaml-opentelemetry#103

I also updated the makefile so semgrep.opam has semgrep.opam.template as a build dependency.

Test plan:
CI Passes

synced from Pro 42be25ccf05a04caaede63f3c87e2c3468864185
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