Add PrometheusReporter implementation#85
Conversation
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
prometheus/src/test/java/com/uber/m3/tally/prometheus/PrometheusReporterTest.java
Outdated
Show resolved
Hide resolved
prometheus/src/test/java/com/uber/m3/tally/prometheus/PrometheusReporterTest.java
Outdated
Show resolved
Hide resolved
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
b2c8831 to
46c8ad6
Compare
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
| registeredCounters.computeIfAbsent(collectorName, key -> Counter.build() | ||
| .name(name) | ||
| .help(String.format("%s counter", name)) | ||
| .labelNames(collectionToStringArray(ttags.keySet())) |
There was a problem hiding this comment.
Re-evaluate all hot-path methods in terms of memory churn: reduce allocations in the hot-path to a minimum and avoid copying static arrays at all costs. Instead pre-initialize static arrays (like buckets, tags, etc) during initialization and re-use it across the board.
There was a problem hiding this comment.
Could you please be more specific? Which hot-path are you referring to? Static buckets for timers are already preinitialized. Tags are not static and passed as arguments to report<Something> methods. All array allocations happen only during the initialization of Prometheus collectors, calculation of canonical metric IDs and passing tags values to Prometheues collectors' collect methods. We cannot do much about it unless rewriting the collectors implementations ourself.
If there is something particular in the implementation, which you think can be improved, please be more specific and I'd love to improve it. As a follow up I'm planning to add benchmark tests for all the implementations of Reporter interface, which would give us more clear picture about performance.
There was a problem hiding this comment.
+1 to both points--yes we should optimize; doing it as a followup with benchmarks makes reasonable sense to me.
@alexeykudinkin on the benchmarking point--any suggestions on good approaches for profiling allocations/garbage creation?
Instead pre-initialize static arrays (like buckets, tags, etc) during initialization and re-use it across the board.
Good call; we could probably do this kind of thing even in absence of good data (though of course, it'd be hard to tell the impact...).
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @RunWith(JUnit4.class) | ||
| public static class ConcurrencyTest { |
There was a problem hiding this comment.
Nice, thanks for adding these! Could you confirm that they don't flake if you run them multiple times? (I tend to do 1000 if they're fast, 100 if they're slow).
There was a problem hiding this comment.
https://stackoverflow.com/questions/1492856/easy-way-of-running-the-same-junit-test-over-and-over suggests just temporarily using a parametrized test for it, or looks like Intellij can just do it in the run config--whatever's easy :)
| for (int i = 0; i < nThreads; i++) { | ||
| Thread t = new Thread(() -> { | ||
| try { | ||
| startGate.await(); |
| } | ||
| } catch (Exception e) { | ||
| if (testCase.expectedException == null) { | ||
| Assert.fail("unexpected exception"); |
There was a problem hiding this comment.
Nit: why not just throw here? You'll get a better error message out of it by default. Also, it gives you an early return here to reduce nesting:
if (expected == null) {
throw e
}
// else
| @Parameterized.Parameters(name = "{index}: {0}}") | ||
| public static Collection<Case> data() { | ||
| return Arrays.asList( | ||
| new Case( |
There was a problem hiding this comment.
Nice, I like this approach.
prometheus/src/test/java/com/uber/m3/tally/prometheus/PrometheusReporterTest.java
Show resolved
Hide resolved
prometheus/src/main/java/com/uber/m3/tally/prometheus/PrometheusReporter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Diff lgtm! To @alexeykudinkin 's point about perf + memory utilization--I definitely agree, but I think it makes reasonable sense to do benchmarks + optimization in a followup diff (which I think is already planned, right @SokolAndrey?).

Adds PrometheusReporter implementation.
The implementation mimics the uber-go/tally Prometheus reporter implementation.
Fixes #72