search

eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

Don't fail the MetricID constructor if MP Config is not available. #466

Closed jmartisk closed 4 years ago

jmartisk commented 5 years ago

Fixes #463

I decided to not do any kind of logging even though this would probably deserve a WARN log, but it's not appropriate in the API code and we don't depend on any particular logging API. So probably it would be an optional task for the implementation to detect this condition and log a warning during start. Perhaps we will be able to log this if the MP Logging spec makes its way out in the future.

donbourne commented 5 years ago

Hm. the serviceability cop in me says this is kind of scary. We don't even have a return code (or field) that the caller can use to determine whether mp config was ignored, so it would be challenging for the caller to help with the logging on this.

Perhaps we need an env var that we could check with values that would basically say "don't use MP config" or "allow to run without MP config". That way we could leave the somewhat descriptive exception in place to warn users about what's wrong, but have a way for them to say "i don't care - run anyway".

jmartisk commented 5 years ago

That sounds perhaps somewhat too complicated for such edge case I would say, no?

I think that if the implementation tries calling ConfigProvider.getConfig() once during initialization procedures and it succeeds/fails, then it can assume that it will keep succeeding/failing the same way for the rest of the application's lifecycle, and therefore can log it if it failed.

donbourne commented 4 years ago

I agree that having an ENV var to control this is probably too complicated. The problem we still have though is there is no way to log the error the first time it occurs.

I'm trying to weigh the good of theoretically making it possible to skip including MP Config as a dependency vs. the bad of potentially making it more difficult to diagnose a problem at runtime.

Have you already tried running without MP Config? Does it just throw a NoClassDefFoundError in the MetricID constructor?

jmartisk commented 4 years ago

It either throws NoClassDefFoundError if Config is not at all on your classpath, or IllegalStateException if the API is present, but it can't find an implementation. I guess if the container just tries to call ConfigProvider.getConfig() during boot it can detect this problem and log it...

Yushan-Lin commented 4 years ago

Runs fine in Liberty!

jmartisk commented 4 years ago

just rebased this and resolved conflicts

donbourne commented 4 years ago

I had an interesting discussion with @NottyCode on this one. He asserts (I'd say correctly) that we probably shouldn't have APIs with implementation in them, and that that's the root of our issue here. Had we made the use of MP Config be something that the implementation was responsible for then it would be easy for the implementation to log appropriately or provide a way to indicate the dependency wasn't needed.

That said, I think Metrics makes such minimal and isolated use of MP Config that I'm ok with this PR's change.

jmartisk commented 4 years ago

+1 that this should probably be responsibility of the implementation. But we do have some (maybe too much) of business logic in the API code (MetricID class in particular). Perhaps we could think whether MetricID could become an interface and it would be the responsibility of the implementation to implement it. But I'm not sure how much of a compatibility impact that would be, maybe too much.