Skip to content

Commit 67a409a

Browse files
pirgeojonatan-ivanov
pirgeo
and
jonatan-ivanov
authored
[Dynatrace v2] Ensure synchronization when taking snapshots (#3615)
* Ensure synchronization when taking snapshots * Change convertIfNecessary parameter order * Fix call sites after parameter order change --------- Co-authored-by: Jonatan Ivanov <[email protected]>
1 parent d2fefd8 commit 67a409a

File tree

7 files changed

+144
-96
lines changed

7 files changed

+144
-96
lines changed

‎implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/types/DynatraceDistributionSummary.java

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,14 +80,18 @@ public double min() {
8080
return summary.getMin();
8181
}
8282

83+
/**
84+
* @deprecated see {@link DynatraceSummarySnapshotSupport#hasValues()}.
85+
*/
8386
@Override
87+
@Deprecated
8488
public boolean hasValues() {
8589
return count() > 0;
8690
}
8791

8892
@Override
8993
public DynatraceSummarySnapshot takeSummarySnapshot() {
90-
return new DynatraceSummarySnapshot(min(), max(), totalAmount(), count());
94+
return summary.takeSummarySnapshot();
9195
}
9296

9397
@Override
@@ -98,9 +102,7 @@ public DynatraceSummarySnapshot takeSummarySnapshot(TimeUnit timeUnit) {
98102

99103
@Override
100104
public DynatraceSummarySnapshot takeSummarySnapshotAndReset() {
101-
DynatraceSummarySnapshot snapshot = takeSummarySnapshot();
102-
summary.reset();
103-
return snapshot;
105+
return summary.takeSummarySnapshotAndReset();
104106
}
105107

106108
@Override

‎implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/types/DynatraceSummary.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,20 @@ long getCount() {
6767
return Double.longBitsToDouble(max.longValue());
6868
}
6969

70+
DynatraceSummarySnapshot takeSummarySnapshot() {
71+
synchronized (this) {
72+
return new DynatraceSummarySnapshot(getMin(), getMax(), getTotal(), getCount());
73+
}
74+
}
75+
76+
DynatraceSummarySnapshot takeSummarySnapshotAndReset() {
77+
synchronized (this) {
78+
DynatraceSummarySnapshot snapshot = takeSummarySnapshot();
79+
reset();
80+
return snapshot;
81+
}
82+
}
83+
7084
void reset() {
7185
synchronized (this) {
7286
min.set(0);

‎implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/types/DynatraceSummarySnapshotSupport.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@
2525
*/
2626
public interface DynatraceSummarySnapshotSupport {
2727

28+
/**
29+
* @deprecated This method might lead to problems with a race condition if values are
30+
* added to the summary after reading the number of values already recorded. Take a
31+
* snapshot and use {@link DynatraceSummarySnapshot#getCount()} instead.
32+
*/
33+
@Deprecated()
2834
boolean hasValues();
2935

3036
DynatraceSummarySnapshot takeSummarySnapshot();

‎implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/types/DynatraceTimer.java

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,13 @@ public DynatraceTimer(Id id, Clock clock, DistributionStatisticConfig distributi
5555
}
5656
}
5757

58+
/**
59+
* @deprecated see {@link DynatraceSummarySnapshotSupport#hasValues()}.
60+
*/
5861
@Override
62+
@Deprecated
5963
public boolean hasValues() {
60-
return count() > 0;
64+
return summary.getCount() > 0;
6165
}
6266

6367
@Override
@@ -67,7 +71,7 @@ public DynatraceSummarySnapshot takeSummarySnapshot() {
6771

6872
@Override
6973
public DynatraceSummarySnapshot takeSummarySnapshot(TimeUnit unit) {
70-
return new DynatraceSummarySnapshot(min(unit), max(unit), totalTime(unit), count());
74+
return convertIfNecessary(summary.takeSummarySnapshot(), unit);
7175
}
7276

7377
@Override
@@ -77,9 +81,17 @@ public DynatraceSummarySnapshot takeSummarySnapshotAndReset() {
7781

7882
@Override
7983
public DynatraceSummarySnapshot takeSummarySnapshotAndReset(TimeUnit unit) {
80-
DynatraceSummarySnapshot snapshot = takeSummarySnapshot(unit);
81-
summary.reset();
82-
return snapshot;
84+
return convertIfNecessary(summary.takeSummarySnapshotAndReset(), unit);
85+
}
86+
87+
DynatraceSummarySnapshot convertIfNecessary(DynatraceSummarySnapshot snapshot, TimeUnit unit) {
88+
if (unit == baseTimeUnit()) {
89+
return snapshot;
90+
}
91+
92+
return new DynatraceSummarySnapshot(unit.convert((long) snapshot.getMin(), baseTimeUnit()),
93+
unit.convert((long) snapshot.getMax(), baseTimeUnit()),
94+
unit.convert((long) snapshot.getTotal(), baseTimeUnit()), snapshot.getCount());
8395
}
8496

8597
@Override

‎implementations/micrometer-registry-dynatrace/src/main/java/io/micrometer/dynatrace/v2/DynatraceExporterV2.java

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -199,17 +199,16 @@ private String createCounterLine(Meter meter, Measurement measurement) {
199199

200200
Stream<String> toTimerLine(Timer meter) {
201201
if (!(meter instanceof DynatraceSummarySnapshotSupport)) {
202-
// fall back to previous behaviour
203202
return toSummaryLine(meter, meter.takeSnapshot(), getBaseTimeUnit());
204203
}
205204

206-
DynatraceSummarySnapshotSupport summary = (DynatraceSummarySnapshotSupport) meter;
207-
if (!summary.hasValues()) {
205+
DynatraceSummarySnapshot snapshot = ((DynatraceSummarySnapshotSupport) meter)
206+
.takeSummarySnapshotAndReset(getBaseTimeUnit());
207+
208+
if (snapshot.getCount() == 0) {
208209
return Stream.empty();
209210
}
210211

211-
// Take a snapshot and reset the Timer for the next export
212-
DynatraceSummarySnapshot snapshot = summary.takeSummarySnapshotAndReset(getBaseTimeUnit());
213212
return createSummaryLine(meter, snapshot.getMin(), snapshot.getMax(), snapshot.getTotal(), snapshot.getCount());
214213
}
215214

@@ -249,18 +248,15 @@ private Stream<String> createSummaryLine(Meter meter, double min, double max, do
249248

250249
Stream<String> toDistributionSummaryLine(DistributionSummary meter) {
251250
if (!(meter instanceof DynatraceSummarySnapshotSupport)) {
252-
// fall back to previous behaviour
253251
return toSummaryLine(meter, meter.takeSnapshot(), null);
254252
}
255253

256-
DynatraceSummarySnapshotSupport summary = (DynatraceSummarySnapshotSupport) meter;
254+
DynatraceSummarySnapshot snapshot = ((DynatraceSummarySnapshotSupport) meter).takeSummarySnapshotAndReset();
257255

258-
if (!summary.hasValues()) {
256+
if (snapshot.getCount() == 0) {
259257
return Stream.empty();
260258
}
261259

262-
// Take a snapshot and reset the DistributionSummary for the next export
263-
DynatraceSummarySnapshot snapshot = ((DynatraceSummarySnapshotSupport) meter).takeSummarySnapshotAndReset();
264260
return createSummaryLine(meter, snapshot.getMin(), snapshot.getMax(), snapshot.getTotal(), snapshot.getCount());
265261
}
266262

‎implementations/micrometer-registry-dynatrace/src/test/java/io/micrometer/dynatrace/types/DynatraceDistributionSummaryTest.java

Lines changed: 32 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -44,23 +44,21 @@ class DynatraceDistributionSummaryTest {
4444
private static final Clock CLOCK = new MockClock();
4545

4646
@Test
47-
void testHasValues() {
47+
void testSummaryCount() {
4848
DynatraceDistributionSummary ds = new DynatraceDistributionSummary(ID, CLOCK, DISTRIBUTION_STATISTIC_CONFIG, 1);
49-
assertThat(ds.hasValues()).isFalse();
49+
50+
assertThat(ds.count()).isZero();
5051
ds.record(3.14);
51-
assertThat(ds.hasValues()).isTrue();
52+
assertThat(ds.count()).isEqualTo(1);
53+
ds.record(5.6);
54+
assertThat(ds.count()).isEqualTo(2);
5255

53-
// reset, hasValues should be initially false
5456
ds.takeSummarySnapshotAndReset();
55-
assertThat(ds.hasValues()).isFalse();
56-
57-
// add invalid value, hasValues stays false
58-
ds.record(-1.234);
59-
assertThat(ds.hasValues()).isFalse();
57+
assertThat(ds.count()).isZero();
6058
}
6159

6260
@Test
63-
void testDynatraceDistributionSummary() {
61+
void testDynatraceDistributionSummaryValuesAreRecorded() {
6462
DynatraceDistributionSummary ds = new DynatraceDistributionSummary(ID, CLOCK, DISTRIBUTION_STATISTIC_CONFIG, 1);
6563
ds.record(3.14);
6664
ds.record(4.76);
@@ -91,56 +89,58 @@ void testRecordNegativeIgnored() {
9189
}
9290

9391
@Test
94-
void testMinMaxAreOverwritten() {
92+
void testUnitsAreIgnored() {
9593
DynatraceDistributionSummary ds = new DynatraceDistributionSummary(ID, CLOCK, DISTRIBUTION_STATISTIC_CONFIG, 1);
96-
ds.record(3.14);
97-
ds.record(4.76);
98-
ds.record(0.123);
99-
ds.record(8.93);
10094

101-
assertMinMaxSumCount(ds, 0.123, 8.93, 16.953, 4);
95+
ds.record(100);
96+
DynatraceSummarySnapshot microsecondsSnapshot = ds.takeSummarySnapshot(TimeUnit.MICROSECONDS);
97+
DynatraceSummarySnapshot daysSnapshot = ds.takeSummarySnapshot(TimeUnit.DAYS);
98+
99+
// both the microseconds and the days snapshot return the same values.
100+
assertMinMaxSumCount(microsecondsSnapshot, daysSnapshot.getMin(), daysSnapshot.getMax(),
101+
daysSnapshot.getTotal(), daysSnapshot.getCount());
102102
}
103103

104104
@Test
105-
void testGetSnapshotNoReset() {
105+
void testUnitsAreIgnoredButResetWorks() {
106106
DynatraceDistributionSummary ds = new DynatraceDistributionSummary(ID, CLOCK, DISTRIBUTION_STATISTIC_CONFIG, 1);
107-
ds.record(3.14);
108-
ds.record(4.76);
109107

110-
assertMinMaxSumCount(ds.takeSummarySnapshot(), 3.14, 4.76, 7.9, 2);
111-
// run twice to make sure it's not reset in between
112-
assertMinMaxSumCount(ds.takeSummarySnapshot(), 3.14, 4.76, 7.9, 2);
108+
ds.record(100);
109+
DynatraceSummarySnapshot microsecondsSnapshot = ds.takeSummarySnapshotAndReset(TimeUnit.MICROSECONDS);
110+
assertMinMaxSumCount(microsecondsSnapshot, 100, 100, 100, 1);
111+
assertMinMaxSumCount(ds.takeSummarySnapshot(TimeUnit.DAYS), 0, 0, 0, 0);
113112
}
114113

115114
@Test
116-
void testGetSnapshotNoResetWithTimeUnitIgnored() {
115+
void testMinMaxAreOverwritten() {
117116
DynatraceDistributionSummary ds = new DynatraceDistributionSummary(ID, CLOCK, DISTRIBUTION_STATISTIC_CONFIG, 1);
118117
ds.record(3.14);
119118
ds.record(4.76);
119+
ds.record(0.123);
120+
ds.record(8.93);
120121

121-
assertMinMaxSumCount(ds.takeSummarySnapshot(TimeUnit.MINUTES), 3.14, 4.76, 7.9, 2);
122-
// run twice to make sure it's not reset in between
123-
assertMinMaxSumCount(ds.takeSummarySnapshot(TimeUnit.MINUTES), 3.14, 4.76, 7.9, 2);
122+
assertMinMaxSumCount(ds, 0.123, 8.93, 16.953, 4);
124123
}
125124

126125
@Test
127-
void testGetSnapshotAndReset() {
126+
void testGetSnapshotNoReset() {
128127
DynatraceDistributionSummary ds = new DynatraceDistributionSummary(ID, CLOCK, DISTRIBUTION_STATISTIC_CONFIG, 1);
129128
ds.record(3.14);
130129
ds.record(4.76);
131130

132-
assertMinMaxSumCount(ds.takeSummarySnapshotAndReset(), 3.14, 4.76, 7.9, 2);
133-
assertMinMaxSumCount(ds.takeSummarySnapshotAndReset(), 0d, 0d, 0d, 0);
131+
assertMinMaxSumCount(ds.takeSummarySnapshot(), 3.14, 4.76, 7.9, 2);
132+
// check the distribution summary was reset.
133+
assertMinMaxSumCount(ds, 3.14, 4.76, 7.9, 2);
134134
}
135135

136136
@Test
137-
void testGetSnapshotAndResetWithTimeUnitIgnored() {
137+
void testGetSnapshotAndReset() {
138138
DynatraceDistributionSummary ds = new DynatraceDistributionSummary(ID, CLOCK, DISTRIBUTION_STATISTIC_CONFIG, 1);
139139
ds.record(3.14);
140140
ds.record(4.76);
141141

142-
assertMinMaxSumCount(ds.takeSummarySnapshotAndReset(TimeUnit.MINUTES), 3.14, 4.76, 7.9, 2);
143-
assertMinMaxSumCount(ds.takeSummarySnapshotAndReset(TimeUnit.MINUTES), 0d, 0d, 0d, 0);
142+
assertMinMaxSumCount(ds.takeSummarySnapshotAndReset(), 3.14, 4.76, 7.9, 2);
143+
assertMinMaxSumCount(ds, 0d, 0d, 0d, 0);
144144
}
145145

146146
private void assertMinMaxSumCount(DynatraceDistributionSummary ds, double expMin, double expMax, double expTotal,

0 commit comments

Comments
 (0)