Skip to content
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

fix: wait for cleanup in cohttp client #76

Merged
merged 2 commits into from
Oct 22, 2024
Merged

fix: wait for cleanup in cohttp client #76

merged 2 commits into from
Oct 22, 2024

Conversation

c-cube
Copy link
Member

@c-cube c-cube commented Oct 17, 2024

in Opentelemetry_client_cohttp_lwt.with_setup we should now wait for
the cleanup to be done, by sneaking in a unit Lwt.u that is only
resolved after the cleanup is done.

close #41

in `Opentelemetry_client_cohttp_lwt.with_setup` we should now wait for
the cleanup to be done, by sneaking in a `unit Lwt.u` that is only
resolved after the cleanup is done.

close #41
@tatchi
Copy link
Contributor

tatchi commented Oct 18, 2024

I tested with the same code as in #41 and cannot reproduce with this change.

I had to slightly adapt the code though due to the breaking API change

diff --git a/experimental/coco/main.ml b/experimental/coco/main.ml
index 1bebf24487b..58aa21e6386 100644
--- a/experimental/coco/main.ml
+++ b/experimental/coco/main.ml
@@ -9,4 +9,4 @@ let () =
   let config =
     Opentelemetry_client_cohttp_lwt.Config.make ~debug:true ~url_traces:"http://127.0.0.1:4318/v1/traces" ()
   in
-  Opentelemetry_client_cohttp_lwt.with_setup ~config () @@ fun () -> Lwt_main.run (run ())
+  Opentelemetry_client_cohttp_lwt.with_setup ~config () run |> Lwt_main.run

You'll have to adapt it as well in the tests:

diff --git a/tests/bin/cohttp_client.ml b/tests/bin/cohttp_client.ml
index dcd395e..8cd4dbb 100644
--- a/tests/bin/cohttp_client.ml
+++ b/tests/bin/cohttp_client.ml
@@ -70,5 +70,4 @@ let () =
     "Check HTTP requests at \
      https://requestbin.com/r/enec1hql02hz/26qShWryt5vJc1JfrOwalhr5vQt@.";
 
-  Opentelemetry_client_cohttp_lwt.with_setup ~config () (fun () ->
-      Lwt_main.run (run ()))
+  Opentelemetry_client_cohttp_lwt.with_setup ~config () run |> Lwt_main.run
diff --git a/tests/bin/emit1_cohttp.ml b/tests/bin/emit1_cohttp.ml
index deb5359..849ff37 100644
--- a/tests/bin/emit1_cohttp.ml
+++ b/tests/bin/emit1_cohttp.ml
@@ -139,5 +139,5 @@ let () =
         Printf.printf "\ndone. %d spans in %.4fs (%.4f/s)\n%!"
           (Atomic.get num_tr) elapsed n_per_sec)
   in
-  Opentelemetry_client_cohttp_lwt.with_setup ~stop ~config () @@ fun () ->
-  Lwt_main.run @@ run ()
+  Opentelemetry_client_cohttp_lwt.with_setup ~stop ~config () run
+  |> Lwt_main.run

@c-cube c-cube marked this pull request as ready for review October 18, 2024 17:18
@c-cube c-cube requested a review from mattjbray as a code owner October 18, 2024 17:18
@c-cube
Copy link
Member Author

c-cube commented Oct 18, 2024

Fixed the tests. So, would you consider this to be an adequate fix for #41?

@c-cube c-cube merged commit 9813ec6 into main Oct 22, 2024
3 checks passed
@c-cube c-cube deleted the simon/fix-41 branch October 22, 2024 02:57
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.

opentelemetry-cohttp: make sure to emit data before termination
3 participants