Closed mspiegel closed 9 years ago
Yeah progress!
I'm not sure if there is any good way to handle the dependnecies. Forcing everyone to have both 2.x and 3.x seems not good. (We might only be annoyed internally but that would probably be a big red flag for Cassandra.) We handle that for the metrics-graphite
and whatnot by setting them as optional and then using reflection to interact with them. But I don't want to write the entire library with reflection.
It looks like the changes aren't as drastic as I feared and I think configs are backwards compatible. rateunit
& duration
unit means 3.x configs might not do what you want with 2.x configs, but a 2.x config with 3.x code will mostly do a reasonable thing.
The least bad thing might be to maintain a 2.x and 3.x branch (with 3.x probably being master). 3x will need to mix up the package name (config3
or something) to co-exist but otherwise patches will have a decent shot at applying to both.
My vote is to have support for both versions in master. It's not too much duplication of code, the project remains fairly small, and having it all in one place makes it easier to maintain. I made the metrics-core2 and metrics-core3 dependencies optional so the downstream user would have to explicitly import the library they want. Another bonus of having a single branch is that it allows downstream users to import one metrics-reporter-config library if they want to emit both metrics 2.x and metrics 3.x in their application at the same time. That bizarre configuration might be used while they are transitioning.
I've renaming the enable() and enable(MetricRegistry) methods to enable2() and enable3(MetricRegistry) respectively. And added enable() as a deprecated method that calls enable2().
I know they are optional in maven but if com.yammer.metrics.core.MetricPredicate
or com.codahale.metrics.MetricFilter
arn't on the classpath won't loading AbstractReporterConfig
(for example) fail on the import statements?
OK second attempt. I've created a submodule for metrics-reporter2 and one for metrics-reporter3. Most of the implementation is shared in the metrics-base submodule. Copy/pasting was necessary for the com.addthis.metrics.reporter.config.ReporterConfig class as each submodule is using its own implementation of the reporters.
Bump. Any thoughts? Too much complexity?
Bump.
Sorry for the very long delay in reviewing this. I think you have convinced me that dual support is feasible. Based on adoption rates I think we will end up wanting to support both for years to come.
Comments:
metrics-core
depdencies listed as optional in the two poms? We can't run without them.src/test/resources
? Diff looks like they are the same which would be a surpsringly succesful amount of backwards compatibility.GMetric.UDPAddressingMode.MULTICAST
is almost certianly not what we want. (We use unicast and that's what the config parser assumes.) I think supporting multicast configs would be a worthy patch if someone wanted to tackle it, but it's not how things work now.ReporterConfig
is the most copy-paste-y looking. Could the 'stuff at the bottom' like validate
, and loadFromFile
and ReporterConfigurationException
be pulled out?metrics3 things I'm still confused about:
Things that can be taken care of post merge:
Merging pull request #13.
A thank you again to @tomvandenberge for that pull request. I was able to integrate it easily into the reporter-config2 package (support for metrics 2.x). There is a design problem in metrics 3.x. All of the reporters have a builder that accepts a Registry but they forgot to add to their parent interface a method that accepts a Registry and generates a builder. They will also need to add an interface for the Builder. I'll submit a pull request to metrics to clean that up and then we can adopt this feature to the reporter-config3 package.
I will continue to clean up this pull request to make it suitable for landing.
Update:
metrics-core
depdencies listed as optional in the two poms? We can't run without them.src/test/resources
? Diff looks like they are the same which would be a surpsringly succesful amount of backwards compatibility.GMetric.UDPAddressingMode.MULTICAST
is almost certianly not what we want. (We use unicast and that's what the config parser assumes.) I think supporting multicast configs would be a worthy patch if someone wanted to tackle it, but it's not how things work now.ReporterConfig
is the most copy-paste-y looking. Could the 'stuff at the bottom' like validate
, and loadFromFile
and ReporterConfigurationException
be pulled out?With regard to "Meters and timers no longer have rate or duration units; those are properties of reporters." The AbstractReporterConfig has two new fields named "rateunit" and "durationunit" with default values of SECONDS and MILLISECONDS, respectively. This makes the default behavior in metrics 3.0 equivalent to the current behavior in metrics 2.0.
@cburroughs I think we are ready to merge to master. Can you look it over once more before the merge?
I went ahead and renamed the package 'com.addthis.metrics' to 'com.addthis.metrics3' in the reporter-config3 module. I foresee a use-case where applications will want to emit both metrics 2.0 and metrics 3.0 metrics because they have some old library that is using metrics 2.0. I think we'll have to do some additional work to make this happen (specifically I'm concerned about ports colliding if we turn on both emitters). But renaming the package for metrics 3.0 is a start towards this goal.
Good catch on the metircs3 package namespace. That's something we need to support. I think this is good to merge.
Would you like me to do that or do you want to go ahead and handle the conflicts?
Merged to master.
This implementation has barely been tested with metrics 2.x or 3.x. Specifically I have been able to run the CSV reporter with metrics 2.x or 3.x. We should probably do some additional testing before merging to master.
I do not have a lot of experience with metrics 3.x. Based on the pull request https://github.com/addthis/metrics-reporter-config/pull/6 and the Metrics 3.x Getting Started Guide https://dropwizard.github.io/metrics/3.1.0/getting-started, it looks like the application is responsible for creating a MetricRegistry object.
Since we used the default registry in the metrics 2.x implementation, I created two parallel APIs. The old API has #enable() methods that do not accept arguments. The new API has #enable(registry) methods that accept a Metrics 3.x MetricRegistry object. I tried to reuse as much code as possible in between the two APIs.
If we want to adopt an API that is more explicit about the version of the metrics library, such as either passing in an enum parameter with the metrics version, or changing the name of the method to enable2() or something like that, I am fine with any of that. The final API should probably be backwards compatible with the existing #enable() methods.
There are three FIXME statements. They do not break this implementation but they are worth addressing.