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

Add example for actix-web framework #53

Closed
oriontvv opened this issue Mar 18, 2022 · 3 comments · Fixed by #76
Closed

Add example for actix-web framework #53

oriontvv opened this issue Mar 18, 2022 · 3 comments · Fixed by #76

Comments

@oriontvv
Copy link
Contributor

oriontvv commented Mar 18, 2022

Hi guys, thanks for your contribution!

There is an issue with using the client with actix-web framework.
Because Registry doesn't implement Copy - the only working solution I've found is to wrap it with Mutex. But it requires locking at least on each collecting of metrics which isn't good for performance. Are there any other suggestions?

@mxinden
Copy link
Member

mxinden commented Mar 21, 2022

Because Registry doesn't implement Copy

I think you are confusing Copy and Clone.

Differs from Copy in that Copy is implicit and an inexpensive bit-wise copy, while Clone is always explicit and may or may not be expensive. In order to enforce these characteristics, Rust does not allow you to reimplement Copy, but you may reimplement Clone and run arbitrary code.

https://doc.rust-lang.org/std/clone/trait.Clone.html

I think a good rule of thumb whether a type should implement Clone is whether it fits into a single CPU cache line. This is not the case for Registry, i.e. Registry even spills to the heap.

the only working solution I've found is to wrap it with Mutex. But it requires locking at least on each collecting of metrics which isn't good for performance.

First I would like to question "isn't good for performance". In case this is happening mostly on a single core, memory synchronization costs are likely low. Asked from a different perspective, how would you like to share an object between concurrent executions without some mechanism of synchronization? We could invent some lock-free / wait-free mechanism, though I doubt that is worth the effort (see below). This is a serious question. Happy for suggestions.

To dive a bit deeper into the matter of performance here, the lock only enforces mutual exclusion on the metric collection path. I would expect this path to never be a bottleneck on your end. In other words I would not expect more than ~1 scrape per second. Note that metric recording can happen concurrently.

Let me know if the above is of some help @oriontvv.

@oriontvv
Copy link
Contributor Author

oriontvv commented Mar 26, 2022

@mxinden Hi, thanks for the feedback and sorry for late reply.
Yes, you are absolutely right - Registry doesn't implement Clone, my mistake, thanks.

Asked from a different perspective, how would you like to share an object between concurrent executions without some mechanism of synchronization? We could invent some lock-free / wait-free mechanism, though I doubt that is worth the effort (see below). This is a serious question. Happy for suggestions.

About the question - I thought that it's possible to avoid blocking like an example for tide do(I don't see any blocking for State)
But Arc can't be used with actix-web(or I do something wrong) - compiler says that:

let registry: Arc<Registry> = Arc::new(Registry::default());
HttpServer::new(move || {
    App::new()
        .app_data(registry.clone())
})
.bind(("127.0.0.1", 8080))?
.run()
.await

96 |     HttpServer::new(move || {
   |     ^^^^^^^^^^^^^^^ `dyn SendEncodeMetric` cannot be shared between threads safely

@mxinden
Copy link
Member

mxinden commented Mar 27, 2022

But Arc can't be used with actix-web(or I do something wrong) - compiler says that:

On vacation right now, though #57 (comment) might help in the meantime.

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 a pull request may close this issue.

2 participants