highly undesirable change in logger behavior

[ripienaar]

In the past users of the gem could plug in custom logger classes with just the methods they wish to get callback logging for.

This is important because different versions of this gem have different callback logging methods and you keep adding more all the time - a good thing. So if there's any hope of maintaining compatibility with any existing code that use custom loggers you have to first check if you can call a certain logger callback.

Previous code was:

      if @logger && @logger.respond_to?(:on_ack)
        @logger.on_ack(log_params, headers)
      end

Now it's just

@logger.on_ack(log_params, headers)

which means anyone who use the gem and who didn't implement every single logger method in their custom loggers will now have critical errors.

[PaulGale]

This is the reason why the NullLogger was introduced.

Your logger should inherit from NullLogger to obtain default 'do nothing' behavior for logging methods you're not interested in handling.

Should new logging methods be introduced they will be added to the NullLogger each time.

[ripienaar]

So everyone has to change their code now to support this new logger behavior? This is a terrible user experience.

It's fine in small little projects, but people who rely on this gem on large publicly used projects can simply not do this.

[PaulGale]

How hard is it to add < Stomp::NullLogger to one's logger class?

If a "large publicly used project" is prepared to make a new release that includes the upgraded gem (which is the only time they'd notice the gem change) then they can add the code above to their logger at the same time.

If one's not ready to absorb the change then don't upgrade the local copy of the gem.

[ripienaar]

You do not vendor the gem, this is against policies of all the distributions and expectations that systems people have. Not everything behaves like a rails site.

So it's actually really hard to get this change out, this is a project used to manage literally 100 000+ machines across 100s of sites, we're seeing thousands of downloads a day of this software. And it's included in yum/apt/etc repos of many projects. This is software used to manage and build things like public clouds and the core of automation infrastructure and thus very hard to upgrade for users. It really just isn't as simple as you think.

This morning already we've had 3 users burned by this change.

[PaulGale]

The popularity of any gem is orthogonal to how dependencies are managed. If one want's to be conservative about one's dependencies then make them explicit. If folks choose not to make their dependencies explicit then they accept the risk that goes with that. Either that or it reflects a flaw in their dependency management strategy. This is unrelated to the channel of distribution whether it be yum or any other method. Upgrading is a choice and not something that happens behind your back. If someone is receiving the latest version of the gem it's because they chose to, either explicitly or implicitly.

[PaulGale]

This morning already we've had 3 users burned by this change.

BTW, who is 'we' in this statement? Just wondering.

[PaulGale]

This morning already we've had 3 users burned by this change.

Are you saying that you pulled in the latest version of the gem into your application then released your application without testing it first? Just wanted to be clear here. I'm guessing so, given the statement '3 users burned by this change' as 'users' appear to be functioning as testers. Like I said, I'm guessing.

[ripienaar]

The other side of that coin is mature release engineering on the part of the gem. Use of semver, changelogs that call out behavior changes like this and so forth and so on. All things that this project now seem to ignore, which unfortunately makes it unusable.

But fine, you obviously feel making huge breaking changes is not your responsibility, leave it at that.

[ripienaar]

We do not vendor the gem, as I said.

[PaulGale]

If you're not 'vendoring'. What I'm referring to is stating the versions of one's dependencies explicitly, regardless of how or where they're pulled in from.

Adopting a strategy of automatically pulling in the latest version of a gem means you accept the risk that goes with that, especially if elect not to regression test prior. If someone is determined to release their software without regression testing it first they only have themselves to blame.

Yes, the change logs could have been more explicit (not that I'm a committer).

The use of the NullLogger does not constitute a 'huge breaking change'. Unit tests on your side would have caught the change within seconds. Do you have some?

[ripienaar]

'huge breaking change' is when existing code now produces an unhandled exception. It's really simple.

[ripienaar]

but fine, like I said, you seem to be more concerned with letting the blame roll off your back, if that's how you roll then rock on.

will just treat this project from now on as one with immature release strategy vs how it was till recently.

[PaulGale]

You're projecting.

[rtyler]

@ripienaar This looks like it's related to @ismith's recent change and subsequent 1.3.0 release. I'm not sure if the Stomp gem already follows the semver convention, but it seems reasonable to do so.

For the record, @ppaul is not a committer, nor am I, so don't judge the project too harshly by his rough edges..

[ismith]

@ripienaar My apologies. The intent was to allow those respond_to? calls to be replaced by having loggers subclass the NullLogger provided; obviously that was premature.

The commit I just pushed to the dev branch puts those back (although the NullLogger is still there, and subclassable); I'll get a release (1.3.1) out with that later today.

[daniel-rikowski]

@ismith Thank you very much!

Perhaps in the future NullLogger can be a module. That way we'd have a little more flexibility in case someone is already inheriting from another class.

Also consciously committing to Semver (http://semver.org/) might prevent similar situations in the future. That way people could still be receiving bug fixes automatically and at the same time be protected from breaking API changes.

[ismith]

Forgot to close this, but 1.3.1 was released earlier today and should resolve this issue.