Replace ImplementationNotAccessibleException with RuntimeException

We're defining two custom exception classes in metrics-lib: DescriptorParseException and ImplementationNotAccessibleException. The first makes sense to me, because applications might want to catch and handle that. In fact they have to, because it's a checked exception.

But I don't see why we have ImplementationNotAccessibleException there. It's only thrown in DescriptorSourceFactory in this method, and it's a runtime exception that applications shouldn't catch anyway. We could easily replace it with a generic RuntimeException with the same message and have one type less in the API.

I'd say that given it's a runtime exception, we can take it out in 1.8.0. But if that's too soon, let's deprecate it now and take it out in 2.0.0.

Thoughts?

changed milestone to %metrics-lib 2.0.0

added component::metrics/library milestone::metrics-lib 2.0.0 owner::metrics-team priority::medium resolution::implemented severity::normal status::closed type::enhancement labels

Metrics-lib provides an API and an implementation. When there are more implementations (either by a third party or less likely by Metrics) the runtime exception ImplementationNotAccessibleException could be used to access another implementation.

I know there are not that many implementations out there, but this is a useful distinction from a generic runtime exception. So, I would like to keep this tiny exception.

Ah, I think I never considered that distinction between API and implementation important. Realistically, we'll provide the only implementation of this API.

(Maybe this also explains why you care about keeping the info log line "Serving implementation {} for {}." whereas I'd still prefer toning that down to a debug message.)

Let's take this ticket out of the 1.8.0 milestone and discuss later whether we really need this abstraction layer or not.

Trac:
Milestone: metrics-lib 1.8.0 to N/A

Replying to karsten:

Ah, I think I never considered that distinction between API and implementation important. Realistically, we'll provide the only implementation of this API.

(Maybe this also explains why you care about keeping the info log line "Serving implementation {} for {}." whereas I'd still prefer toning that down to a debug message.)

Let's take this ticket out of the 1.8.0 milestone and discuss later whether we really need this abstraction layer or not.

We briefly discussed this topic when wondering what to put in the announcement of 2.0.0 next week. I'm adding here what I wrote there:

But let's also discuss whether we even want to make that distinction for others to write their own metrics-lib implementations. (We have over a week to finalize this blog post, so we're not in a big rush.)

The main reason for having separate packages for interfaces and implementation was that application developers should not even bother about implementation details, they should not even see them. Basically, if they import an impl class in their code, they're doing it wrong. Just like Java developers shouldn't depend on sun.* classes.

But when I created those two packages, I never thought about developers implementing our interfaces and providing them as their own impl classes. That's certainly possible and I wouldn't want to make this unnecessarily difficult. But is that also realistic, and is that something we want to sell as feature? It's not like the Java Security API with a reference implementation.

Going one step back and looking at the big Metrics picture, I see how we're providing quite a few interfaces: we're providing raw descriptors in CollecTor, we're providing a descriptor-parsing library with metrics-lib, we're providing aggregated CSV files, and we're providing the Onionoo API. Is that not enough?

Again, I'm not trying to change anything. I'm just a bit careful, because I wouldn't want to promise yet another interface, tied to the guarantee of not messing up this interface without sufficient prior notice.

But maybe I'm just overcautious. Curious to hear your thoughts!

You agreed with these concerns and added two related aspects that I'm paraphrasing here: If we really want to provide an API, we'll also have to provide an API test framework, and we'll have to establish a process for accepting or rejecting implementations.

So, I think we agree here. Does that mean we should move forward with the suggestion above? In theory, it's a trivial change that could still go into 1.9.0. Adding to that milestone just so that we consider it.

Trac:
Milestone: N/A to metrics-lib 1.9.0
Status: new to needs_review

We're planning to put out 1.9.0 tomorrow, and I'm now creating a pre-release tarball and testing it in various applications. This change doesn't seem important enough to delay this. Moving out of planned milestones.

Trac:
Milestone: metrics-lib 1.9.0 to N/A

Well, the test frame work would be useful on its own (e.g. for API changes).

Maybe, have a quick check if there is some RuntimeException that fits closer than vanilla runtime ex.? On first sight: ProviderNotFoundException? But, in general, ok I'll give up one that :-)

The 'serving implementation' log statement could be shifted to debug level. That way there is a mechanism we can use when implementing and after releasing changes like the switch of the DescriptorCollector implementation.

Okay! :) ProviderNotFoundException sounds good enough to me. Optimistically moving to 2.0.0, because neither of the two changes (exception and log level) should require much prior notice.

Trac:
Milestone: N/A to metrics-lib 2.0.0

Meh. ProviderNotFoundException doesn't have a constructor that accepts a Throwable. Maybe we shouldn't pick unrelated runtime exceptions and simply use RuntimeException. After all, we don't want people to catch them and distinguish them by type anyway, and the message will be sufficient to track down the issue.

I also took out the log message change and linked the discussion here in #20430 (moved). Let's tackle that together with other logging changes.

Please review my branch task-22476 for 2.0.0.

Ok, the patch looks fine; I can live with RuntimeException ;-) Open question moved to #20430 (moved), fine.

All checks and tests pass. Ready for 2.0.0!

Trac:
Status: needs_review to merge_ready

Great, thanks for looking! Merged to master. Closing.

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

closed