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

impl Clone for Collector might be a footgun #28

Open
caelunshun opened this issue May 22, 2024 · 3 comments
Open

impl Clone for Collector might be a footgun #28

caelunshun opened this issue May 22, 2024 · 3 comments

Comments

@caelunshun
Copy link

I recently tracked down a bug in some code caused by my incorrect assumption about Collector::clone: I assumed it would behave like an Arc in maintaining a reference to the underlying data structure, which (I thought) is a common pattern in Rust crates dealing with concurrency. This, of course, was incorrect and led to objects being mixed between separate collectors.

I see now that the docs for Collector::clone mention this behavior, but rustdoc doesn't do a great job of showing docs for trait implementations. I wondered if it would be less likely to cause confusion if the cloning behavior was in a differently named method, e.g. clone_config.

This is just a suggestion. Maybe I'm the only one who would make this assumption about clone, not sure.

@ibraheemdev
Copy link
Owner

ibraheemdev commented May 23, 2024

Yes, I completely agree. I was actually meaning to change this a while ago but it fell off my radar. I think we should probably just remove the Clone implementation, and add with_config(self, config: Config) and config(&self) -> Config methods that can be used to easily recreate collector configuration (where Config holds the current epoch_frequency and batch_size settings).

@ibraheemdev
Copy link
Owner

Removed in 97ef579.

@ibraheemdev
Copy link
Owner

Reopening this until the fix releases, it's not part of the latest release as it is a breaking change.

@ibraheemdev ibraheemdev reopened this Nov 27, 2024
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

No branches or pull requests

2 participants