Skip to content

Commit c5b5e77

Browse files
treintrein
committed
Fix Snapshot computation in TestScope
The following changes have been made as part of this commit: 1. Refactored how snapshots are created to leverage the same reporting mechanism used in regular metrics recording. 2. Fixed snapshots for Timers. The previous TimerImpl implementation relied on NoReporterSink to properly create TimerSnapshots. However given TestScope instances are built with NullStatsReporter, TimerSnapshots were not being created at all when using TestScope. 3. Leveraged the ImmutableBuckets implementation to compute bucket bounds in HistogramImpl, allowing removal of a substantial amount of duplicated code. 4. Added unit tests to cover the fixes for TimerSnapshots as well as the changes to the snapshot logic.
1 parent cccc649 commit c5b5e77

19 files changed

Lines changed: 580 additions & 418 deletions

‎core/src/main/java/com/uber/m3/tally/CounterImpl.java‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,30 +42,34 @@ public void inc(long delta) {
4242
curr.getAndAdd(delta);
4343
}
4444

45+
@Override
46+
public void report(ImmutableMap<String, String> tags, StatsReporter reporter) {
47+
long delta = snapshot();
48+
if (reporter instanceof SnapshotBasedStatsReporter) {
49+
// Always report snapshots.
50+
reporter.reportCounter(getQualifiedName(), tags, delta);
51+
} else if (delta != 0) {
52+
// Only report deltas if they are non-zero. NOTE: we call value() here to update the previous value.
53+
reporter.reportCounter(getQualifiedName(), tags, value());
54+
}
55+
}
56+
57+
/**
58+
* Returns the delta between the current and previous values. NOTE: This method has side effects.
59+
*/
4560
long value() {
4661
long current = curr.get();
4762
long previous = prev.get();
48-
4963
if (current == previous) {
5064
return 0;
5165
}
52-
5366
prev.set(current);
54-
5567
return current - previous;
5668
}
5769

58-
@Override
59-
public void report(ImmutableMap<String, String> tags, StatsReporter reporter) {
60-
long delta = value();
61-
62-
if (delta == 0) {
63-
return;
64-
}
65-
66-
reporter.reportCounter(getQualifiedName(), tags, delta);
67-
}
68-
70+
/**
71+
* Returns the difference between the current and previous values without mutating counters.
72+
*/
6973
long snapshot() {
7074
return curr.get() - prev.get();
7175
}

‎core/src/main/java/com/uber/m3/tally/CounterSnapshotImpl.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,9 @@
2828
* Default implementation of a {@link CounterSnapshot}.
2929
*/
3030
class CounterSnapshotImpl implements CounterSnapshot {
31-
private String name;
32-
private ImmutableMap<String, String> tags;
33-
private long value;
31+
private final String name;
32+
private final ImmutableMap<String, String> tags;
33+
private final long value;
3434

3535
CounterSnapshotImpl(String name, ImmutableMap<String, String> tags, long value) {
3636
this.name = name;

‎core/src/main/java/com/uber/m3/tally/GaugeImpl.java‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@
2929
* Default implementation of a {@link Gauge}.
3030
*/
3131
class GaugeImpl extends MetricBase implements Gauge, Reportable {
32-
private AtomicBoolean updated = new AtomicBoolean(false);
33-
private AtomicLong curr = new AtomicLong(0);
32+
private final AtomicBoolean updated = new AtomicBoolean(false);
33+
private final AtomicLong curr = new AtomicLong(0);
3434

3535
protected GaugeImpl(ScopeImpl scope, String fqn) {
3636
super(fqn);
@@ -44,18 +44,14 @@ public void update(double value) {
4444
updated.set(true);
4545
}
4646

47-
double value() {
48-
return Double.longBitsToDouble(curr.get());
49-
}
50-
5147
@Override
5248
public void report(ImmutableMap<String, String> tags, StatsReporter reporter) {
5349
if (updated.getAndSet(false)) {
5450
reporter.reportGauge(getQualifiedName(), tags, value());
5551
}
5652
}
5753

58-
double snapshot() {
59-
return value();
54+
double value() {
55+
return Double.longBitsToDouble(curr.get());
6056
}
6157
}

‎core/src/main/java/com/uber/m3/tally/HistogramImpl.java‎

Lines changed: 28 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,7 @@
2424
import com.uber.m3.util.ImmutableMap;
2525

2626
import java.util.Collections;
27-
import java.util.HashMap;
2827
import java.util.List;
29-
import java.util.Map;
3028

3129
/**
3230
* Default implementation of a {@link Histogram}.
@@ -79,9 +77,9 @@ private CounterImpl getOrCreateCounter(int index) {
7977
}
8078

8179
List<?> bucketsBounds =
82-
this.type == Type.VALUE
83-
? specification.getValueUpperBounds()
84-
: specification.getDurationUpperBounds();
80+
this.type == Type.VALUE
81+
? specification.getValueUpperBounds()
82+
: specification.getDurationUpperBounds();
8583

8684
// To maintain lock granularity we synchronize only on a
8785
// particular bucket leveraging bucket's boundary as a sync target
@@ -124,99 +122,58 @@ public Stopwatch start() {
124122
return new Stopwatch(clock.nowNanos(), this);
125123
}
126124

127-
ImmutableMap<String, String> getTags() {
128-
return tags;
129-
}
130-
131-
private Duration getUpperBoundDurationForBucket(int bucketIndex) {
132-
return bucketIndex < specification.getDurationUpperBounds().size() ? specification.getDurationUpperBounds().get(bucketIndex) : Duration.MAX_VALUE;
133-
}
134-
135-
private Duration getLowerBoundDurationForBucket(int bucketIndex) {
136-
return bucketIndex == 0 ? Duration.MIN_VALUE : specification.getDurationUpperBounds().get(bucketIndex - 1);
137-
}
138-
139-
private double getUpperBoundValueForBucket(int bucketIndex) {
140-
return bucketIndex < specification.getValueUpperBounds().size() ? specification.getValueUpperBounds().get(bucketIndex) : Double.MAX_VALUE;
141-
}
142-
143-
private double getLowerBoundValueForBucket(int bucketIndex) {
144-
return bucketIndex == 0 ? Double.MIN_VALUE : specification.getValueUpperBounds().get(bucketIndex - 1);
145-
}
146-
147-
private long snapshotCounterValue(int index) {
148-
return bucketCounters[index] != null ? bucketCounters[index].snapshot() : 0;
149-
}
150-
151-
// NOTE: Only used in testing
152-
Map<Double, Long> snapshotValues() {
153-
if (type == Type.DURATION) {
154-
return null;
155-
}
156-
157-
Map<Double, Long> values = new HashMap<>(bucketCounters.length, 1);
158-
159-
for (int i = 0; i < bucketCounters.length; ++i) {
160-
values.put(getUpperBoundValueForBucket(i), snapshotCounterValue(i));
161-
}
162-
163-
return values;
164-
}
165-
166-
Map<Duration, Long> snapshotDurations() {
167-
if (type == Type.VALUE) {
168-
return null;
169-
}
170-
171-
Map<Duration, Long> durations = new HashMap<>(bucketCounters.length, 1);
172-
173-
for (int i = 0; i < bucketCounters.length; ++i) {
174-
durations.put(getUpperBoundDurationForBucket(i), snapshotCounterValue(i));
175-
}
176-
177-
return durations;
178-
}
179-
180125
@Override
181126
public void recordStopwatch(long stopwatchStart) {
182127
recordDuration(Duration.between(stopwatchStart, clock.nowNanos()));
183128
}
184129

185-
enum Type {
130+
/**
131+
* Returns the tags associated with this histogram.
132+
*/
133+
ImmutableMap<String, String> getTags() {
134+
return tags;
135+
}
136+
137+
private enum Type {
186138
VALUE,
187139
DURATION
188140
}
189141

190142
/**
191-
* Extension of the {@link CounterImpl} adjusting it's reporting procedure
192-
* to adhere to histogram format
143+
* Extension of the {@link CounterImpl} adjusting its reporting procedure to adhere to histogram format.
193144
*/
194-
class HistogramBucketCounterImpl extends CounterImpl {
145+
private class HistogramBucketCounterImpl extends CounterImpl {
195146

196147
private final int bucketIndex;
197148

198-
protected HistogramBucketCounterImpl(ScopeImpl scope, String fqn, int bucketIndex) {
149+
private HistogramBucketCounterImpl(ScopeImpl scope, String fqn, int bucketIndex) {
199150
super(scope, fqn);
200151

201152
this.bucketIndex = bucketIndex;
202153
}
203154

204155
@Override
205156
public void report(ImmutableMap<String, String> tags, StatsReporter reporter) {
206-
long inc = value();
207-
if (inc == 0) {
208-
// Nothing to report
209-
return;
157+
long inc = snapshot();
158+
if (reporter instanceof SnapshotBasedStatsReporter) {
159+
// Always report snapshots.
160+
reportBucket(tags, reporter, inc);
161+
} else if (inc != 0) {
162+
// Only report when there is a change in the counter. NOTE: we call value() here to
163+
// update the previous value.
164+
reportBucket(tags, reporter, value());
210165
}
166+
}
211167

168+
private void reportBucket(ImmutableMap<String, String> tags, StatsReporter reporter, long inc) {
212169
switch (type) {
213170
case VALUE:
214171
reporter.reportHistogramValueSamples(
215172
getQualifiedName(),
216173
tags,
217174
(Buckets) specification,
218-
getLowerBoundValueForBucket(bucketIndex),
219-
getUpperBoundValueForBucket(bucketIndex),
175+
specification.getValueLowerBoundFor(bucketIndex),
176+
specification.getValueUpperBoundFor(bucketIndex),
220177
inc
221178
);
222179
break;
@@ -225,8 +182,8 @@ public void report(ImmutableMap<String, String> tags, StatsReporter reporter) {
225182
getQualifiedName(),
226183
tags,
227184
(Buckets) specification,
228-
getLowerBoundDurationForBucket(bucketIndex),
229-
getUpperBoundDurationForBucket(bucketIndex),
185+
specification.getDurationLowerBoundFor(bucketIndex),
186+
specification.getDurationUpperBoundFor(bucketIndex),
230187
inc
231188
);
232189
break;

‎core/src/main/java/com/uber/m3/tally/HistogramSnapshotImpl.java‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,27 +23,22 @@
2323
import com.uber.m3.util.Duration;
2424
import com.uber.m3.util.ImmutableMap;
2525

26+
import java.util.HashMap;
2627
import java.util.Map;
28+
import java.util.concurrent.ConcurrentHashMap;
2729

2830
/**
2931
* Default implementation of a {@link HistogramSnapshot}.
3032
*/
3133
class HistogramSnapshotImpl implements HistogramSnapshot {
32-
private String name;
33-
private ImmutableMap<String, String> tags;
34-
private Map<Double, Long> values;
35-
private Map<Duration, Long> durations;
34+
private final String name;
35+
private final ImmutableMap<String, String> tags;
36+
private final Map<Double, Long> values = new ConcurrentHashMap<>();
37+
private final Map<Duration, Long> durations = new ConcurrentHashMap<>();
3638

37-
HistogramSnapshotImpl(
38-
String name,
39-
ImmutableMap<String, String> tags,
40-
Map<Double, Long> values,
41-
Map<Duration, Long> durations
42-
) {
39+
HistogramSnapshotImpl(String name, ImmutableMap<String, String> tags) {
4340
this.name = name;
4441
this.tags = tags;
45-
this.values = values;
46-
this.durations = durations;
4742
}
4843

4944
@Override
@@ -58,11 +53,25 @@ public Map<String, String> tags() {
5853

5954
@Override
6055
public Map<Double, Long> values() {
61-
return values;
56+
return new ImmutableMap<>(values);
6257
}
6358

6459
@Override
6560
public Map<Duration, Long> durations() {
66-
return durations;
61+
return new ImmutableMap<>(durations);
62+
}
63+
64+
/**
65+
* Appends a new duration to the current snapshot. We kept the access modifier as default to avoid any external access.
66+
*/
67+
void addDuration(Duration upperBound, long samples) {
68+
durations.put(upperBound, samples);
69+
}
70+
71+
/**
72+
* Appends a new value to the current snapshot. We kept the access modifier as default to avoid any external access.
73+
*/
74+
void addValue(double upperBound, long samples) {
75+
values.put(upperBound, samples);
6776
}
6877
}

‎core/src/main/java/com/uber/m3/tally/ScopeBuilder.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ protected ScopeBuilder(ScheduledExecutorService scheduler, ScopeImpl.Registry re
6868
}
6969

7070
/**
71-
* Update the reporter
71+
* Updates the reporter.
7272
* @param reporter value to update to
7373
* @return Builder with new param updated
7474
*/
@@ -78,7 +78,7 @@ public ScopeBuilder reporter(StatsReporter reporter) {
7878
}
7979

8080
/**
81-
* Update the prefix
81+
* Updates the prefix.
8282
* @param prefix value to update to
8383
* @return Builder with new param updated
8484
*/
@@ -88,7 +88,7 @@ public ScopeBuilder prefix(String prefix) {
8888
}
8989

9090
/**
91-
* Update the separator
91+
* Updates the separator.
9292
* @param separator value to update to
9393
* @return Builder with new param updated
9494
*/
@@ -98,7 +98,7 @@ public ScopeBuilder separator(String separator) {
9898
}
9999

100100
/**
101-
* Update the tags, cloning the tags map to an ImmutableMap
101+
* Updates the tags, cloning the tags map to an ImmutableMap.
102102
* @param tags value to update to
103103
* @return Builder with new param updated
104104
*/
@@ -109,7 +109,7 @@ public ScopeBuilder tags(Map<String, String> tags) {
109109
}
110110

111111
/**
112-
* Update the tags. Since this function takes an ImmutableMap, we don't need to clone it
112+
* Updates the tags. Since this function takes an ImmutableMap, we don't need to clone it.
113113
* @param tags value to update to
114114
* @return Builder with new param updated
115115
*/
@@ -120,7 +120,7 @@ public ScopeBuilder tags(ImmutableMap<String, String> tags) {
120120
}
121121

122122
/**
123-
* Update the defaultBuckets
123+
* Updates the defaultBuckets.
124124
* @param defaultBuckets value to update to
125125
* @return Builder with new param updated
126126
*/
@@ -146,7 +146,7 @@ ScopeImpl build() {
146146
}
147147

148148
/**
149-
* Creates a root scope and starts reporting with the specified interval
149+
* Creates a root scope and starts reporting with the specified interval.
150150
* @param interval duration between each report
151151
* @return the root scope created
152152
*/
@@ -155,7 +155,7 @@ public Scope reportEvery(Duration interval) {
155155
}
156156

157157
/**
158-
* Creates a root scope and starts reporting with the specified interval
158+
* Creates a root scope and starts reporting with the specified interval.
159159
* @param interval duration between each report
160160
* @param uncaughtExceptionHandler an {@link java.lang.Thread.UncaughtExceptionHandler} that's
161161
* called when there's an uncaught exception in the report loop

0 commit comments

Comments
 (0)