-
Notifications
You must be signed in to change notification settings - Fork 13
Fix Eio collector to work accross domains #103
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ee1ce7e to
f355287
Compare
shonfeder
commented
Aug 1, 2025
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.
Contributor
Author
|
The test failure here is just a networking error: https://github.com/imandra-ai/ocaml-opentelemetry/actions/runs/16682099040/job/47222974012?pr=103#step:6:263 |
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
c-cube
approved these changes
Sep 2, 2025
Member
c-cube
left a comment
There was a problem hiding this 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.
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.tis not thread-safe, and cannot be used across domains, but the implementation passed a switch into theCollector.Backend, which is then accessed across domains:ocaml-opentelemetry/src/client-cohttp-eio/opentelemetry_client_cohttp_eio.ml
Lines 476 to 479 in 46d1289
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:
emit1_eioto produce deterministic signals even when run across multiple domains.