5.1.1 introduces backward incompatible change

[tomas-langer]

Method bucketValues does not exist in 5.0.0 specification of org.eclipse.microprofile.metrics.Snapshot, causing that a class extending Snapshot is not compilable when upgrading.

New abstract methods (on abstract clases or interfaces) should always be created with default implementation, otherwise this is a backward incompatible change and should be done in a major version.

    /**
     * Returns an array of {@link HistogramBucket} containing the bucket and associated value of this {@link Snapshot}
     * at the moment invocation.
     *
     * @return an array of {@link HistogramBucket} if it is available or an empty array if not available
     */
    public abstract HistogramBucket[] bucketValues();

To reproduce:

1. Use MP Metrics 5.0.0
2. Create a class that extends org.eclipse.microprofile.metrics.Snapshot
3. Compile the project
4. Upgrade dependency to MP Metrics 5.1.1
5. Try to re-compile the project
   The compilation fails with

MyImplementation is not abstract and does not override abstract method bucketValues() in org.eclipse.microprofile.metrics.Snapshot

[tjquinno]

@Channyboy @Emily-Jiang As I mentioned in the MP metrics gitter channel I have a few changes to address this locally but I get bnd baseline plug-in failures in removing abstract and adding an implementation of the method.

[Emily-Jiang]

From this use case, it seems that the class Snapshot is a consumer interface (expect customers to implement this interface). For consumer interfaces, adding any extra abstract method should cause a major version update. Therefore, you have observed the backward incompatible changes.

Strictly speaking, service release should not have API changes. However, since this is a challenge and this change should not affect the current working system, it might be okay to do the change. @tjquinno you will need to update the package-info.java and the bundle versions because of the method update. Here is the PR I provided. However, it will have to be 5.2 release. I guess the spec team needs to discuss how to proceed with this. @Channyboy @tjquinno @donbourne please discuss.

[donbourne]

@tomas-langer , while an application technically could implement the Snapshot interface, there is no way to make MP Metrics implementations use a customer's custom implementation of a Snapshot interface, so in practice it is only implemented by vendors as part of their MP Metrics implementation. So I believe that the steps you've listed to recreate this are only something vendors working on providing an MP Metrics impl would encounter.

@Emily-Jiang , based on my previous comment, this feels like an API bug that only affects vendors working on MP Metrics implementation.

[donbourne]

...and if we agree it only affects MP Metrics implementations (which would have to provide an implementation for that method to comply with the spec), then it isn't even really a bug.

[tjquinno]

We have some Helidon users who provided their own implementation of Snapshot in their library code. Their attempt to build their library using the earliest release of Helidon which depends on MP Metrics 5.1.1 failed with the error Tomas cited, and that led Tomas to file this issue.

To get a better understanding of their use case I am in the process of exploring with those users exactly why they've provided their own Snapshot subclass and how they use their implementation.

That said, it can be reasonably argued that the revision of the Snapshot abstract class in 5.1.1 should have provided an implementation of this new method that returns an empty array (which is specified as the default) so as to maintain strict backward compatibility of the types in MP Metrics according to semantic versioning, notwithstanding your (valid) point, Don, that practically speaking only an implementation of MP Metrics would be concerned. Although the implementors we knew about might have said that they were OK with this (strictly speaking) incompatible change, there is always the chance (quite unlikely but non-zero) of an implementor we do not know about!

[tjquinno]

All this said, if MP Metrics ever creates a new release to address #795 or any other issue, it would be simple for us to address this one as well.

[donbourne]

That said, it can be reasonably argued that the revision of the Snapshot abstract class in 5.1.1 should have provided an implementation of this new method that returns an empty array (which is specified as the default) so as to maintain strict backward compatibility of the types in MP Metrics according to semantic versioning, notwithstanding your (valid) point, Don, that practically speaking only an implementation of MP Metrics would be concerned. Although the implementors we knew about might have said that they were OK with this (strictly speaking) incompatible change, there is always the chance (quite unlikely but non-zero) of an implementor we do not know about!

I agree with your points on that. I'm interested to hear what the Helidon users are doing with it, when you find out.

[Azquelt]

To clarify, this method was introduced in 5.1.0 (i.e. in a minor release), not 5.1.1 (which would be a service release). That's an important distinction and the title of the issue should be updated.

While a default implementation could have been added, is Snapshot really an interface that users should be implementing? Generally adding new methods to interfaces that users don't implement in a minor release is acceptable, isn't it?

I'd like to hear more about why not having a default implementation of this method is a breaking change for the user.

[tjquinno]

Our user had written a private no-op implementation of MP Metrics (I believe as a performance optimization). It's in a library that their downstream applications use.

So, when Helidon adopted MP Metrics 5.1.1 the incompatible change in the MP Snapshot abstract class caused their compilation to fail.

In this case, our "user" is not an application developer. I agree that's it would be unusual for an application developer to implement Snapshot but this use case is different from that.

[donbourne]

We discussed in the MP Technical call today that we would create a 5.1.2 release to add a default implementation to the new Snapshot method.

It was discussed that this is a very borderline case -- if a consumer of an API is behaving like a provider of an impl of an API (for example, creating a noop implementation of the API), then they should expect to be broken when either the major or minor version changes (per semantic versioning rules - https://docs.osgi.org/whitepaper/semantic-versioning/Semantic-Versioning-20190110.pdf )

[Channyboy]

@tomas-langer Can we close this issue now?