search

metrics-rs / metrics

A metrics ecosystem for Rust.
MIT License
1.08k stars 148 forks source link

0.22 Breaking Change: metrics::set_global_recorder takes ownership of the `metrics::Recorder` object #437

Closed bmorin closed 7 months ago

bmorin commented 7 months ago

I looked into updating my metrics-cloudwatch-embedded crate from 0.21.1 to 0.22

metrics::set_recorder was renamed to metrics::set_global_recorder It went from taking a reference to a &'static dyn to taking ownership of the struct implementing metrics::Recorder. Now the entire struct must be Send.

Is there a reason metrics needs to take ownership of the Recorder object instead of receiving an interface that's threadsafe and 'static?

I appreciate the hard work everyone volunteers, but between this and the breaking macro changes, going from 0.21.1 to 0.22 felt like a lot of breakage for a crate with 9 million downloads and counting.

tobz commented 7 months ago

Is there a reason metrics needs to take ownership of the Recorder object instead of receiving an interface that's threadsafe and 'static?

As part of fixing #409, we now take ownership of the recorder so that if another global recorder is already set, we can return the recorder we were given instead of leaking it.

Also, practically, passing in a &'static reference to a recorder is uncommon: most recorders aren't simple enough to be constructed statically. It was really just a holdover from the initial skeleton of metrics being based on the log crate, which did/does the same thing.

Now the entire struct must be Send.

Interesting. Do you have an MVCE of this? If so, it'd be good to open up an issue.

There isn't anything that should explicitly require Send to be implemented for a recorder implementation, except perhaps by virtue of how it's passed around prior to installation.

I appreciate the hard work everyone volunteers, but between this and the breaking macro changes, going from 0.21.1 to 0.22 felt like a lot of breakage for a crate with 9 million downloads and counting.

I'm sorry you feel that way. The latest release was aimed to reduce the number of breaking changes that users saw, since implementors of exporters can amortize the cost of updating much more effectively. These are all things we're doing prior to a 1.0 release precisely in order to avoid breaking changes in the future.

bmorin commented 7 months ago

Interesting. Do you have an MVCE of this? If so, it'd be good to open up an issue.

Sure, I can write one up.

I'm sorry you feel that way. The latest release was aimed to reduce the number of breaking changes that users saw, since implementors of exporters can amortize the cost of updating much more effectively. These are all things we're doing prior to a 1.0 release precisely in order to avoid breaking changes in the future.

Pulling increment_counter, etc. with no deprecation warning did break my users, all my examples, etc.

metrics_cloudwatch Is on 0.16 despite having been updated somewhat recently.

Really looking forward to 1.0 because interface stability is an issue taking a dependency on this crate.

tobz commented 7 months ago

Pulling increment_counter, etc. with no deprecation warning did break my users, all my examples, etc.

Really looking forward to 1.0 because interface stability is an issue taking a dependency on this crate.

Both of these statements underline the reality of the crate: I'm essentially the only contributor when it comes to design and direction. Things move slowly, and so deprecation cycles, certainly pre-1.0, are a luxury I can't afford.

We get some users (people using the macros, basically) and some exporter implementors (folks like you) who chime in every so often to ask questions, or point out bugs, and so on... but nobody that's been willing to offer up large chunks of time to contribute meaningfully to the design/direction of the crate. If that ever changes, then so might stability guarantees.