Load metrics factory service from classpath if available

[jpkrohling]

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Closes #339

[codecov]

Codecov Report

Merging #433 into master will increase coverage by 0.29%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #433      +/-   ##
============================================
+ Coverage     87.15%   87.45%   +0.29%     
- Complexity      510      512       +2     
============================================
  Files            65       65              
  Lines          1978     1985       +7     
  Branches        258      259       +1     
============================================
+ Hits           1724     1736      +12     
+ Misses          164      160       -4     
+ Partials         90       89       -1

Impacted Files	Coverage Δ	Complexity Δ
.../src/main/java/io/jaegertracing/Configuration.java	91.3% <100%> (+0.65%)	39 <2> (+2)	⬆️
...ava/io/jaegertracing/reporters/RemoteReporter.java	85.71% <0%> (+2.38%)	7% <0%> (ø)	⬇️
...in/java/io/jaegertracing/senders/ThriftSender.java	79.06% <0%> (+4.65%)	10% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ffaf9d...9a82c6a. Read the comment docs.

[objectiser]

Do we need an env var to skip? Wouldn't it be good enough to just not include the dependency on the metrics factory?

[jpkrohling]

We never know what sort of crazy use cases people have. For instance, there might be someone extending our Micrometer Metrics Factory to build their own factory, so, they would have both dependencies on the classpath.

[pavolloffay]

Isn't it better to use ENV to define impl which would you like to load?

[jpkrohling]

The most appropriate thing to do for a Java application is to try to load implementations from the classpath using the service loader. The env var/property here is to skip the service loader entirely. Requiring an env var to specify an implementation isn't really idiomatic and wouldn't be the right direction, IMO.

[pavolloffay]

Maybe I didn't express well myself... I would never require conf property to specify the impl...

With ServiceLoader I have seen patterns (e.g jax-rs clientbuilder resolution) that it's possible to specify implementation by fully qualified class name in case if there are multiple impl on classpath. It's useful for test purposes or just when you have multiple implementations on class path and you want to choose a specific one.

So In this case, if there are multiple impls on classpath I would expect a way how I can specify the impl I want... Or if the env/prop is defined let's load it if not pick the first(random!) one.

[jpkrohling]

I see now what you mean.

How about we keep this idea as a separate issue and see whether this is actually needed? While the code for this would be easy to implement (Class.forName(), basically), it's more code, and it's a code we aren't really sure will be used.

[pavolloffay]

I see that as a common practice rather than skip parameter. I am +1 on removing skip parameter and if necessary add parameter with FQCN in a separate PR (if you don't want to handle it here)

[jpkrohling]

Alright, the latest commit just removed the skip parameter.

[pavolloffay]

There is also missing some documentation in readme how to use this with configuration.

[jpkrohling]

There is also missing some documentation in readme how to use this with configuration.

Changes to README added

[pavolloffay]

I have yet another comment :/.

What about moving service loader method to a separate class? Then users can use with programmatic API.

class MicricsFactoryProvider {
 
  MetricsFactory newMetricsFactory() {
     // service loader
   }
}
```

And other suggestion. The service loader classes are usually in `spi` package. We could define `interface MicricsFactoryProvider` which would be used to provide the implementation rather then binding it directly to `MetricsFactory`

[jpkrohling]

What about moving service loader method to a separate class? Then users can use with programmatic API.

That sounds reasonable for a "core" feature, but I don't think this is the case for the internal client metrics. I'm not sure enough people will care about it and will just be bloat :-/

[pavolloffay]

I mean something like this https://stackoverflow.com/a/2956981/4158442. It's a nice design

[jpkrohling]

I mean something like this https://stackoverflow.com/a/2956981/4158442. It's a nice design

It's indeed a nice design and would probably make sense for core parts of the API. This PR is about a marginal feature.

Let's implement what we need first. If people need a more fully featured SPI for the internal metrics, it's easy add in the future.

[jpkrohling]

I'm merging this as is and will let the discussion about a fully featured SPI loading mechanism as part of #436