search

metrics-rs / metrics

A metrics ecosystem for Rust.
MIT License
1.06k stars 145 forks source link

Make `Snapshotter` `Clone` #486

Closed blp closed 1 month ago

blp commented 1 month ago

Snapshotter is just a wrapper around an Arc and I don't know why it can't be Cloned. I suspect it's just an oversight. It would be convenient to make it easy to pass it around multiple places.

tobz commented 1 month ago

I think the original thought was that it's not really meant to be used for anything except tests, where cloning it would be... I don't know why you would ever have to clone it in a test, basically.

There's no technical reason it can't be Clone, but what's the usecase here?

blp commented 1 month ago

I have multiple tests, so I used a OnceLock to make a globally accessible Snapshotter so that I could use it from all of them, and then I discovered that I couldn't clone it.

For now, I just wrapped Snapshotter in another Arc, which also works OK.

tobz commented 1 month ago

A better approach (IMO) would be to use with_local_recorder, which is designed specifically for tests in order to avoid having to do tricks with a global recorder.

blp commented 1 month ago

I assumed that with_local_recorder wouldn't work with additional threads spawned by the test (but I haven't tried it).

tobz commented 1 month ago

Ah, yeah, it wouldn't work for anything other than the thread that with_local_recorder is called on.

In that case, would be happy to review a PR adding Clone to Snapshotter.

tobz commented 1 month ago

Oh, woops! Just realized this will be implemented once #472 is released.

blp commented 1 month ago

Oh, woops! Just realized this will be implemented once #472 is released.

Oh, perfect, thank you!

tobz commented 1 month ago

Late response, but: this was released in metrics-util@v0.17.0.