Renaming of multi? to quiet? can be breaking
larouxn opened this issue · 11 comments
👋 Just a heads up issue that the recent 3.1.1 patch version release contained a rename of multi? to quiet? which was a breaking change (no method error) for a service I was running. Not sure if that will be the case for many apps/services but just wanted to give a heads up. Maybe we can add a note to the History.md warning people of potential breakage.
Let me check on that. I thought I'd aliased all the public methods
@larouxn So multi? wasn't a method on Dalli::Client. How is your service utilizing this method? Are you directly accessing the Dalli::Protocol::Binary?
@petergoldstein, the calls are indeed directly on Dalli::Protocol::Binary. Still trying to track down exactly what is making the multi? calls to see if it can either stop making calls on Binary or address the outdated method name.
NoMethodError · undefined method `multi?' for #<Dalli::Protocol::Binary:0x00007fd81ed3e4d0>
If it's a public gem making the multi? calls, I hope we can address the issue. Will report back here if I find anything.
👋 Alright, I was able to find the multi? method usage. Thankfully just an internal library, not in a public gem so this may be a false alarm re: breaking change. Granted, other people outside my organization may also be using that method directly, in which case would still be breaking for them. Maybe still worth noting.
@larouxn Ok, the multi? method on Dalli::Protocol::Binary was private. It's definitely not something that users of the library should be calling in general. Can you share why your internal library was calling it?
It's used in an instrumentation library that I'm looking at for the first time right now. Seems to be used to ensure we don't instrument Dalli call latency during multi calls such as multi_get as that would be statistically inaccurate compare to non-multi methods calls. Seems normal multi is also used for "quiet operations".
Hoping to tag someone with more context into here.
Hey @petergoldstein we're doing something similar for dalli auto instrumentation in the opentelemetry-ruby repo.
Ideally we (Otel ruby folks) would love to push for first party instrumentation in the dalli gem itself (let me know if you're open to this idea, I would be happy to contribute PRs towards this).
However I don't want to try and pull this issue onto a tangent, would you be open to supplying a public method for performing this check so we could stop doing unsavoury things like relying on a private method?
@robertlaurin So addressing this in order:
- Yes, I'd be open to including first party instrumentation, provided the output is suitably generic and not tied to particular implementations. If you could put an answer here, that would be a great start. And PRs would be welcome.
- I am open to moving the
quiet?method to public, and maintaining that for the 3.x version. I can even add an alias formulti?.
One note about all of this is that the quiet/multi block is not the only place where we use quiet memcached commands (which is a little confusing). Pipelined gets use quiet memcached commands without setting the thread context variable used by quiet?, so that may impact your instrumentation.
@robertlaurin and @larouxn Take a look at this - #866 . Let me know if that simplifies things for you. I'm ok packaging this and releasing as a 3.1.2 today.
If you could put an answer here, that would be a great start. And PRs would be welcome.
Will do!
I am open to moving the quiet? method to public, and maintaining that for the 3.x version. I can even add an alias for multi?.
That is very helpful, thank you!
One note about all of this is that the quiet/multi block is not the only place where we use quiet memcached commands (which is a little confusing). Pipelined gets use quiet memcached commands without setting the thread context variable used by quiet?, so that may impact your instrumentation.
I'll have to take a closer look at various places I know this being used to see what impact it could have on the accuracy on our instrumentation. Appreciate you flagging that.
@robertlaurin @larouxn I've merged #866 and released 3.1.2 with that included. Hopefully that addresses the issue here. Please let me know if it doesn't.