MetricsRegistry must not use default methods

[rmannibucau]

not sure why but in 3.x the registry added default (convenient I guess) methods to get gauges, timers, counters, ...

However the default impl just enabled stackoverflow.

Since MP provide specs vendors must impl there is no point except pitfalls (as seen in JSON-P already) to use default methods.

More generally, spec must not impl anything and let vendors do.

This issue is about dropping all default methods for the final release.

[jmartisk]

I tend to agree with this, as far as I remember my opinion was kinda outvoted by @jbee and @donbourne Including any implementation snippets in the API is something we should generally avoid. I don't see too much harm in forcing implementations to do a few more lines of code themselves.

[jbee]

However the default impl just enabled stackoverflow.

Elaborate please.

[rmannibucau]

@jbee the default impl delegates to another method (which potentially delegates to another one etc). So it assumes a method is implemented by the vendor but this is an assumption which can be true or false at 50-50. Was likely true for smallrye-metrics (?) but was wrong for geronimo-metrics ending in a stackoverflow cause the default was looping in terms of call cause the impl did the same for another method. Generally speaking default methods are never a good idea for spec and just a dirty way to break users weirdly when upgrading versions.

[jbee]

Default methods certainly don't remove the need to check that all methods are implemented in a consistent way. This includes default methods. Also they are defaults. If an implementation is implemented in a way that requires them to be implemented differently they can and must override them. You make it sound as if default methods lead to some "lock in" that an implementation could not solve itself and would need the api to change. This specifically is not the case. Any implementation can override default methods should they turn out to be problematic or even incorrect. And if they bug you you are free to make it your personal rule to always override any default method present in the api in your implementation and you basically get what you ask for. No difference for you but those people that are fine with defaults don't have to redo the logic.

[rmannibucau]

@jbee you just explained that the spec contains part of the implementation of smallrye, which is not the goal of a spec which must NOT container any implementation, therefore this is a design bug.

Keep in mind it:

1. imposes more work to implementation since it hides unimplemented methods
2. creates nasty user bugs when the user mix an api and impl not compatible (upgrading version automatically with maven or gradle)

So the question I'm asking you is: what does it bring to the spec? Nothing, so why doing that? Should be reverted for the 3.0.0 IMHO.

[jbee]

So the question I'm asking you is: what does it bring to the spec?

It brings clarity. It explains in code what the exact semantics of those methods are specifically given the plurality of similar methods and it avoids implementations to get even more lengthy doing something that semantically is already defined in api context. If your implementation actually gets wrong because counter(String, Tag[]) is not the same as counter(MetricID) although new MetricID(String, Tag[]) is supposed to be semantically the same as the String, Tag[] arguments to counter you have the problem of inconsistent semantics. If on the other hand this is a valid contract there cannot be harm in relying on this contract to be true even in the api.

I don't argue that any default method is unproblematic. I argue that these specific ones are not problematic as they only do add a continence version of a contract that must be considered valid in order for the spec to make sense as it is. This is on the same level as there being a MetricID(String) and a MetricID(String, Tag[]) except that you can override the default method while you cannot override the continence constructor.

[rmannibucau]

It brings clarity

Hmm, did you misunderstand me? I'm not asking what the method as an API brings but what the default with an impl in a spec brings?

So concretely I don't care much duplicating x3 the exact same semantic (metrics spec has still a design bug between metricsid and metadata but we can live with it) but I care about the fact the spec goes beyond its responsability doing an implementation, as small as it is, it is still not under the scope of the spec to do that. In that respect it is a bug.

[jbee]

Hmm, did you misunderstand me?

I think I did understand you. What I mean is if you start to wonder why there are several overloaded variants of a method the default methods give you the answers: These are just for your continence. Using just String or String with Tag[] in the end is the same as using MetricID. That is clarity provided by the code of the default methods. If you skip doing this at least I would start to wonder if there is any semantic difference for the methods since would this only be for convenience the api could have expressed exactly that by implementing them as default methods.

[rmannibucau]

@jbee I got why there are multiple signatures, I don't get why there are default impl - and to be honest it can't be justified at spec level.

[jbee]

I think it is clear that your opinion on default methods is a different one then mine. I tried to explain why I would use them and why the change added them. Rest I think is for the maintainers to decide; what line of thought resonates more with their own.

[rmannibucau]

@jbee well it is just a fact and key point is we are speaking of a spec, not a vendor library or implementation where default methods could make sense. At spec level you can't assume which of the methods is the primary one (there is no reason to priviledge one) and it is nasty to make user deployment fail with stackoverflow instead of NoSuchMethod errors. It is a bad practise which hit JSON-P the first and since then I thought it was known it was not a practise specs must use. The role of the spec is to ensure it is well defined to guarantee the behavior is portable but let enough liberty to all vendors to implement all the parts of the spec the way they want. defaults methods are against that since they define a direction in API without a direction functionally + in terms of API signatures it changes the methods whereas it is out of scope of the deliveries of the spec.

[jbee]

@rmannibucau It remains an opinion about the dos and don'ts of specs. JSON-P might have drawn that conclusion. Who knows how general or transferable this is. I don't want to argue about any of that. I provided the reasoning for my decision to use defaults so however decides can include my reasoning in theirs.

[rmannibucau]

@jbee well, not really, if specs become implementations then there is no more need of vendors and Microprofile becomes an eclipse project as any other. I'm not saying it is bad but just not the goal today from what I understood. Just to give you one, maybe more explicit, example of why it is bad: JSON-P implemented part of the pointer manipulation logic in the spec in last release, it makes it impossible to add any optimization (cache, precomputation etc) in implementations, locking vendors to the spec jar and if there is a bug a new spec must be released. it is a bit less the case in the registry since impl is simpler but it is the same from a design perspective so I think it is important to fix it and spread the word it is a bad practise.

[jbee]

Whatever JSON-P did that required a new release to allow vendors to fix the issue cannot be default methods as they vendors could do overrides if the issue would be there. Same for cache or precomputation. Make an override. Not a lock in situation. The global tags handing in MetricID is a case of implementation in API that should not be there. You cannot make overrides or add a cache.

[rmannibucau]

@jbee agree MetricID class is clearly worse than the registry but it the registry is also an issue for end users not relying on full MP server preassembled (and they are numerous). It prevents them to handle their versions as usual which is a big drawback for enterprises. At spec + vendors level it costs nothing to ease their life so let's do it.

[donbourne]

if I have 3 methods...

void enable() {}
void disable() {}

and

void enable(boolean b) {}

then it doesn't seem harmful for the first two methods to have default impls that call the third method, particularly since 1) the impl is trivial, 2) if you don't like the impl you can override it, 3) presumably there is a testcase that calls the methods to keep you from missing the possible stackoverflow if your impl has the third method call the first two

I don't have a strong opinion either way on this. Given the defaults under discussion are used for new methods it's unlikely the stackoverflow problem would occur in this instance, but if people feel defaults are just bad practice I'm fine not having them too.

[jmartisk]

One more point of view is this: My understanding of default methods in general is that they're basically a hack. Hack that was introduced in Java mostly to allow library developers (and the JDK itself) evolve their interfaces over time without necessarily breaking their implementations. It's a hack because interfaces were designed to not contain any code, they should be just interfaces. If we wanted 'default' code, we should have stayed with an abstract class with some concrete methods.

If were extending an interface within a minor release, we'd need to use default methods. But we're doing this in a major release, so I think we don't need to resort to using them.

[rmannibucau]

@donbourne it does not look for an impl, keep in mind we are speaking about a spec. Also it is actually harmful, some user will proxy with the jdk proxy api these things for various reasons, defaults methods are just not proxyable portably. So only drawbacks and not a single gain to use it here :). It was introduced for backward compatibility in the JVM, MP does not need that and still breaks API elsewhere so no reason to abuse of them.

[jbee]

My understanding of default methods in general is that they're basically a hack

You asked the Java architects about their hack?

interfaces were designed to not contain any code, they should be just interfaces.

And now they are designed to have code in them. Does not make them less interfacy.

Also it is actually harmful, some user will proxy with the jdk proxy api these things for various reasons, defaults methods are just not proxyable portably.

Sure they are. Its just an awful lot of reflection that would be needed to do that. Also not necessary as any proxy could "override" them so the situation is the same as if they had been abstract.

If you want to remove the default methods that is totally valid choice to make. It would just be nice if this is not based or sold on the premise of false believes. What you hold is an opinion and its totally fine to remove them because of that instead of making false claims.

[rmannibucau]

@jbee Java didnt have the choice to do that hack to avoid to create dynamic metaprogramming which is not desired yet, in their case it was justified.

Sure they are. Its just an awful lot of reflection that would be needed to do that. Also not necessary as any proxy could "override" them so the situation is the same as if they had been abstract.

Yes and no, you need to be version bound (and not only java 8, 11 but 8uX 8uY etc) + JVM dependent (hotspot, j9 etc). And please provide me the code which enable to override them, you can't ;). Keep in mind several features are done through reflect proxy to not be bound to a technology, you just broke it all.

Finally which false claim is done? Everything said in this thread is proven since years and you are not able to show it is wrong, if so, please do, I would be happy to be wrong but not a single lib managed to solved that without using asm which is a workaround.

[jbee]

defaults methods are just not proxyable portably

Not true. Reflection allows to find out the method is not implemented by the proxied class, to find the interface that implements it and to invoke the default method of the interface. Or if the proxied class overrides it call that method. That gives you the same behaviour as calling the proxied class directly.

[rmannibucau]

@jbee can you show me how using official java API and not this kind of hack https://github.com/Talend/component-runtime/blob/master/component-runtime-impl/src/main/java/org/talend/sdk/component/runtime/reflect/Defaults.java?

[jbee]

Here https://www.jrebel.com/blog/java-proxies-default-methods-and-method-handles is an article about this. Nobody said using the API java has for this would result in nice code. The claim was it cannot be done and it can.

Anyhow, you made a much better argument by noting that many projects use metrics in a non vendor environment that is sensible to exact artefact versions combined which makes behaviour hard to predict in presence of default methods. In my opinion this the best argument so far.

[rmannibucau]

@jbee just a last message on defaults (don't want to create a thread on that since I guess we agree on the tcehnical part now): the post you shared does not work on multiple JVM versions and it is not a supported feature of the JVM so can break anytime (it did 4 times - at least - since java 8) in new minor java versions so my statement was valid saying there is no supported way to do that, in particular in user land (it is fine for vendors to do this kind of workarounds, not for users). Guess it was just a phrasing thing but I really want to highlight the fact we are speaking of a spec and not a piece of code of a final application, overall quality of the deliverable is way more important cause it is included in much more systems with much more java and OS versions so we should ensure we are very portable.

Anyway, back to the topic, anyone against dropping default and keeping interfaces pures now?

[donbourne]

I'm ok with having no defaults. The benefit of pre-filling trivial methods is tiny, so probably not worth any risks defaults bring.

[rmannibucau]

Think I didn't forget any: https://github.com/eclipse/microprofile-metrics/pull/573