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 signals handling to allow graceful applications' shutdown #24

Closed
wants to merge 1 commit into from

Conversation

neslinesli93
Copy link

When using this library within an actix 4 application, we face the following issue: actix/actix-web#2638

Basically if the application has the actix_web::main macro in the entrypoint, it's not able to handle signals to shutdown gracefully. The hack to solve this is to replace the former macro with tokio::main.

However this issue has been resolved in the opentelemetry repo, where the maintainers suggested to replace opentelemetry::runtime::Tokio with opentelemetry::runtime::TokioCurrentThread when initializing the tracer. I've done a couple tests locally and it seems to work exactly the same, either when tracing is initialized within an actix web context, as well as when it's initialized inside a pure tokio context.

I please ask you to review the change carefully because I don't have any deep knowledge with tokio, nor opentelemetry stuff (or rust at all :D)

@@ -27,7 +27,7 @@ required-features = ["prima-logger-json"]

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
[dependencies]
opentelemetry = { version = "0.16", features = ["rt-tokio"], optional = true }
opentelemetry = { version = "0.16", features = ["rt-tokio-current-thread"], optional = true }
Copy link
Contributor

@miterst miterst Feb 28, 2022

Choose a reason for hiding this comment

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

Maybe we can gate it under a feature so it can work with both actix-web::main(single-threaded) and tokio::main(can be both single/multi threaded)?
Default can be as it's now, and we can add the rt-tokio-current-thread feature.
An example of how opentelemetry-jaeger does it.

Copy link
Author

Choose a reason for hiding this comment

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

It makes a lot of sense! I still have doubts on why during my tests I managed to make it work with both actix_web and tokio main macro... Probably I didn't know how to make it fail :D

Copy link
Contributor

@miterst miterst Feb 28, 2022

Choose a reason for hiding this comment

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

Actually opentelemetry::runtime::TokioCurrentThread would work with both just that its better to make use of the underlying multi-threaded runtime in case we already have it.

Copy link
Author

Choose a reason for hiding this comment

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

At this point, maybe it would be better to pass some kind of option to prima_tracing.rs so that it chooses what kind of strategy to use, instead of a crate feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry my answer above doesn't seem fully right.
I saw that opentelemetry::runtime::TokioCurrentThread does std::thread::spawn so if you use it with the tokio multi-threaded runtime you may end up with both tasks spawned on the same thread and have the same problem.
So in this case it may be better to follow the approach of other similar libraries that expose these options with features.

@neslinesli93
Copy link
Author

Closing for staleness, we'll be using tokio::main and forget about signals :)

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