From ffb731cbb2a9921cd0ec206090883b5a3b75aca9 Mon Sep 17 00:00:00 2001 From: Todd W Crone Date: Wed, 23 Feb 2022 19:40:53 -0500 Subject: [PATCH 1/8] Synchronize on a lock object --- .../agent/stats/ResponseTimeStatsImpl.java | 128 +++++++++++------- .../com/newrelic/agent/stats/StatsImpl.java | 106 +++++++++------ 2 files changed, 140 insertions(+), 94 deletions(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java index 903b631c55..8d095918cf 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java @@ -23,6 +23,8 @@ public class ResponseTimeStatsImpl extends AbstractStats implements ResponseTime private static final long NANOSECONDS_PER_SECOND_SQUARED = TimeConversion.NANOSECONDS_PER_SECOND * TimeConversion.NANOSECONDS_PER_SECOND; + private final Object lock = new Object(); + private long total; private long totalExclusive; private long minValue; @@ -36,12 +38,14 @@ protected ResponseTimeStatsImpl() { @Override public Object clone() throws CloneNotSupportedException { ResponseTimeStatsImpl newStats = new ResponseTimeStatsImpl(); - newStats.count = count; - newStats.total = total; - newStats.totalExclusive = totalExclusive; - newStats.minValue = minValue; - newStats.maxValue = maxValue; - newStats.sumOfSquares = sumOfSquares; + synchronized (lock) { + newStats.count = count; + newStats.total = total; + newStats.totalExclusive = totalExclusive; + newStats.minValue = minValue; + newStats.maxValue = maxValue; + newStats.sumOfSquares = sumOfSquares; + } return newStats; } @@ -67,94 +71,116 @@ public void recordResponseTimeInNanos(long responseTime) { public void recordResponseTimeInNanos(long responseTime, long exclusiveTime) { double responseTimeAsDouble = responseTime; responseTimeAsDouble *= responseTimeAsDouble; - sumOfSquares += responseTimeAsDouble; - if (count > 0) { - minValue = Math.min(responseTime, minValue); - } else { - minValue = responseTime; - } - count++; - total += responseTime; - maxValue = Math.max(responseTime, maxValue); - totalExclusive += exclusiveTime; - if (NewRelic.getAgent().getConfig().getValue(AgentConfigImpl.METRIC_DEBUG, AgentConfigImpl.DEFAULT_METRIC_DEBUG)) { - if (count < 0 || total < 0 || totalExclusive < 0 || sumOfSquares < 0) { - NewRelic.incrementCounter("Supportability/ResponseTimeStatsImpl/NegativeValue"); - Agent.LOG.log(Level.INFO, "Invalid count {0}, total {1}, totalExclusive {2}, or sum of squares {3}", - count, total, totalExclusive, sumOfSquares); + synchronized (lock) { + sumOfSquares += responseTimeAsDouble; + if (count > 0) { + minValue = Math.min(responseTime, minValue); + } else { + minValue = responseTime; + } + count++; + total += responseTime; + maxValue = Math.max(responseTime, maxValue); + totalExclusive += exclusiveTime; + if (NewRelic.getAgent().getConfig().getValue(AgentConfigImpl.METRIC_DEBUG, AgentConfigImpl.DEFAULT_METRIC_DEBUG)) { + if (count < 0 || total < 0 || totalExclusive < 0 || sumOfSquares < 0) { + NewRelic.incrementCounter("Supportability/ResponseTimeStatsImpl/NegativeValue"); + Agent.LOG.log(Level.INFO, "Invalid count {0}, total {1}, totalExclusive {2}, or sum of squares {3}", + count, total, totalExclusive, sumOfSquares); + } } } - } @Override public boolean hasData() { - return count > 0 || total > 0 || totalExclusive > 0; + boolean hasData; + synchronized (lock) { + hasData = count > 0 || total > 0 || totalExclusive > 0; + } + return hasData; } @Override public void reset() { - count = 0; - total = totalExclusive = minValue = maxValue = 0; - sumOfSquares = 0; + synchronized (lock) { + count = 0; + total = totalExclusive = minValue = maxValue = 0; + sumOfSquares = 0; + } } @Override public float getTotal() { - return (float) total / TimeConversion.NANOSECONDS_PER_SECOND; + synchronized (lock) { + return (float) total / TimeConversion.NANOSECONDS_PER_SECOND; + } } @Override public float getTotalExclusiveTime() { - return (float) totalExclusive / TimeConversion.NANOSECONDS_PER_SECOND; + synchronized (lock) { + return (float) totalExclusive / TimeConversion.NANOSECONDS_PER_SECOND; + } } + @Override public float getMaxCallTime() { - return (float) maxValue / TimeConversion.NANOSECONDS_PER_SECOND; + synchronized (lock) { + return (float) maxValue / TimeConversion.NANOSECONDS_PER_SECOND; + } } @Override public float getMinCallTime() { - return (float) minValue / TimeConversion.NANOSECONDS_PER_SECOND; + synchronized (lock) { + return (float) minValue / TimeConversion.NANOSECONDS_PER_SECOND; + } } @Override public double getSumOfSquares() { - return sumOfSquares / NANOSECONDS_PER_SECOND_SQUARED; + synchronized (lock) { + return sumOfSquares / NANOSECONDS_PER_SECOND_SQUARED; + } } @Override public final void merge(StatsBase statsObj) { if (statsObj instanceof ResponseTimeStatsImpl) { ResponseTimeStatsImpl stats = (ResponseTimeStatsImpl) statsObj; - if (stats.count > 0) { - if (count > 0) { - minValue = Math.min(minValue, stats.minValue); - } else { - minValue = stats.minValue; + synchronized (lock) { + if (stats.count > 0) { + if (count > 0) { + minValue = Math.min(minValue, stats.minValue); + } else { + minValue = stats.minValue; + } } - } - count += stats.count; - total += stats.total; - totalExclusive += stats.totalExclusive; + count += stats.count; + total += stats.total; + totalExclusive += stats.totalExclusive; - maxValue = Math.max(maxValue, stats.maxValue); - sumOfSquares += stats.sumOfSquares; + maxValue = Math.max(maxValue, stats.maxValue); + sumOfSquares += stats.sumOfSquares; + } } } @Override public void recordResponseTime(int count, long totalTime, long minTime, long maxTime, TimeUnit unit) { - long totalTimeInNanos = TimeUnit.NANOSECONDS.convert(totalTime, unit); - this.count = count; - this.total = totalTimeInNanos; - this.totalExclusive = totalTimeInNanos; - this.minValue = TimeUnit.NANOSECONDS.convert(minTime, unit); - this.maxValue = TimeUnit.NANOSECONDS.convert(maxTime, unit); - double totalTimeInNanosAsDouble = totalTimeInNanos; - totalTimeInNanosAsDouble *= totalTimeInNanosAsDouble; - sumOfSquares += totalTimeInNanosAsDouble; + synchronized (lock) { + long totalTimeInNanos = TimeUnit.NANOSECONDS.convert(totalTime, unit); + this.count = count; + this.total = totalTimeInNanos; + this.totalExclusive = totalTimeInNanos; + this.minValue = TimeUnit.NANOSECONDS.convert(minTime, unit); + this.maxValue = TimeUnit.NANOSECONDS.convert(maxTime, unit); + double totalTimeInNanosAsDouble = totalTimeInNanos; + totalTimeInNanosAsDouble *= totalTimeInNanosAsDouble; + sumOfSquares += totalTimeInNanosAsDouble; + } } @Override diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java index ec71e9c35e..f036a4dd31 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java @@ -18,6 +18,8 @@ */ public class StatsImpl extends AbstractStats implements Stats { + private final Object lock = new Object(); + private float total; private float minValue; private float maxValue; @@ -39,11 +41,13 @@ public StatsImpl(int count, float total, float minValue, float maxValue, double @Override public Object clone() throws CloneNotSupportedException { StatsImpl newStats = new StatsImpl(); - newStats.count = count; - newStats.total = total; - newStats.minValue = minValue; - newStats.maxValue = maxValue; - newStats.sumOfSquares = sumOfSquares; + synchronized (lock) { + newStats.count = count; + newStats.total = total; + newStats.minValue = minValue; + newStats.maxValue = maxValue; + newStats.sumOfSquares = sumOfSquares; + } return newStats; } @@ -58,84 +62,100 @@ public void recordDataPoint(float value) { if (Float.isNaN(value) || Float.isInfinite(value)) { throw new IllegalArgumentException("Data points must be numbers"); } - double sos = sumOfSquares + (value * value); - if (sos < sumOfSquares) { - throw new IllegalArgumentException("Data value " + value + " caused sum of squares to roll over"); - } - if (count > 0) { - minValue = Math.min(value, minValue); - } else { - minValue = value; - } - count++; - total += value; - maxValue = Math.max(value, maxValue); - sumOfSquares = sos; + synchronized (lock) { + double sos = sumOfSquares + (value * value); + if (sos < sumOfSquares) { + throw new IllegalArgumentException("Data value " + value + " caused sum of squares to roll over"); + } + if (count > 0) { + minValue = Math.min(value, minValue); + } else { + minValue = value; + } + count++; + total += value; + maxValue = Math.max(value, maxValue); + sumOfSquares = sos; - if (NewRelic.getAgent().getConfig().getValue(AgentConfigImpl.METRIC_DEBUG, AgentConfigImpl.DEFAULT_METRIC_DEBUG)) { - if (count < 0 || total < 0) { - NewRelic.incrementCounter("Supportability/StatsImpl/NegativeValue"); - Agent.LOG.log(Level.INFO, "Invalid count {0} or total {1}", count, total); + if (NewRelic.getAgent().getConfig().getValue(AgentConfigImpl.METRIC_DEBUG, AgentConfigImpl.DEFAULT_METRIC_DEBUG)) { + if (count < 0 || total < 0) { + NewRelic.incrementCounter("Supportability/StatsImpl/NegativeValue"); + Agent.LOG.log(Level.INFO, "Invalid count {0} or total {1}", count, total); + } } } - } @Override public boolean hasData() { - return count > 0 || total > 0; + synchronized (lock) { + return count > 0 || total > 0; + } } @Override public void reset() { - count = 0; - total = minValue = maxValue = 0; - sumOfSquares = 0; + synchronized (lock) { + count = 0; + total = minValue = maxValue = 0; + sumOfSquares = 0; + } } @Override public float getTotal() { - return total; + synchronized (lock) { + return total; + } } @Override public float getTotalExclusiveTime() { - return total; + synchronized (lock) { + return total; + } } @Override public float getMinCallTime() { - return minValue; + synchronized (lock) { + return minValue; + } } @Override public float getMaxCallTime() { - return maxValue; + synchronized (lock) { + return maxValue; + } } @Override public double getSumOfSquares() { - return sumOfSquares; + synchronized (lock) { + return sumOfSquares; + } } @Override public void merge(StatsBase statsObj) { if (statsObj instanceof StatsImpl) { StatsImpl stats = (StatsImpl) statsObj; - if (stats.count > 0) { - if (count > 0) { - minValue = Math.min(minValue, stats.minValue); - } else { - minValue = stats.minValue; + synchronized (lock) { + if (stats.count > 0) { + if (count > 0) { + minValue = Math.min(minValue, stats.minValue); + } else { + minValue = stats.minValue; + } } - } - count += stats.count; - total += stats.total; + count += stats.count; + total += stats.total; - maxValue = Math.max(maxValue, stats.maxValue); - sumOfSquares += stats.sumOfSquares; + maxValue = Math.max(maxValue, stats.maxValue); + sumOfSquares += stats.sumOfSquares; + } } } - } From a1e4ffa258138f31a426972d92e8ad9451542a0d Mon Sep 17 00:00:00 2001 From: Todd W Crone Date: Mon, 28 Feb 2022 15:10:49 -0500 Subject: [PATCH 2/8] Add GTSE ticket number for this SNAPSHOT build --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index 733be2d3de..cb36acf60e 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ # The agent version. -agentVersion=7.6.0 +agentVersion=7.6.0-13353 newrelicDebug=false org.gradle.jvmargs=-Xmx2048m From e85c7ce8dd5bcdcf55e76bb6c89744cc452912da Mon Sep 17 00:00:00 2001 From: Todd W Crone Date: Thu, 3 Mar 2022 15:34:34 -0500 Subject: [PATCH 3/8] Add encapsulation and thread safety to AbstractStats --- .../newrelic/agent/stats/AbstractStats.java | 15 +++++++------ .../agent/stats/ResponseTimeStatsImpl.java | 22 +++++++++---------- .../com/newrelic/agent/stats/StatsImpl.java | 22 +++++++++---------- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/stats/AbstractStats.java b/newrelic-agent/src/main/java/com/newrelic/agent/stats/AbstractStats.java index 0e38548157..c5bea390d1 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/stats/AbstractStats.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/stats/AbstractStats.java @@ -11,6 +11,7 @@ import java.io.Writer; import java.util.Arrays; import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; import org.json.simple.JSONArray; @@ -24,7 +25,7 @@ public abstract class AbstractStats implements CountStats { Number zero = 0; ZERO_ARRAY_LIST = Arrays.asList(zero, zero, zero, zero, zero, zero); } - protected int count; + private final AtomicInteger count = new AtomicInteger(0); /** * Used when we want to send up a metric with a zero call count. Not available in the public api. @@ -61,33 +62,33 @@ public AbstractStats() { public AbstractStats(int count) { super(); - this.count = count; + this.count.set(count); } @Override public void incrementCallCount(int value) { - count += value; + this.count.addAndGet(value); } @Override public void incrementCallCount() { - this.count++; + this.count.incrementAndGet(); } @Override public int getCallCount() { - return count; + return count.get(); } @Override public void setCallCount(int count) { - this.count = count; + this.count.set(count); } @Override public final void writeJSONString(Writer writer) throws IOException, InvalidStatsException { List list; - if (count < 0) { + if (count.get() < 0) { list = ZERO_ARRAY_LIST; } else { list = Arrays.asList(count, getTotal(), getTotalExclusiveTime(), getMinCallTime(), diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java index 8d095918cf..813077ae19 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java @@ -39,7 +39,7 @@ protected ResponseTimeStatsImpl() { public Object clone() throws CloneNotSupportedException { ResponseTimeStatsImpl newStats = new ResponseTimeStatsImpl(); synchronized (lock) { - newStats.count = count; + newStats.setCallCount(this.getCallCount()); newStats.total = total; newStats.totalExclusive = totalExclusive; newStats.minValue = minValue; @@ -73,20 +73,20 @@ public void recordResponseTimeInNanos(long responseTime, long exclusiveTime) { responseTimeAsDouble *= responseTimeAsDouble; synchronized (lock) { sumOfSquares += responseTimeAsDouble; - if (count > 0) { + if (getCallCount() > 0) { minValue = Math.min(responseTime, minValue); } else { minValue = responseTime; } - count++; + incrementCallCount(); total += responseTime; maxValue = Math.max(responseTime, maxValue); totalExclusive += exclusiveTime; if (NewRelic.getAgent().getConfig().getValue(AgentConfigImpl.METRIC_DEBUG, AgentConfigImpl.DEFAULT_METRIC_DEBUG)) { - if (count < 0 || total < 0 || totalExclusive < 0 || sumOfSquares < 0) { + if (getCallCount() < 0 || total < 0 || totalExclusive < 0 || sumOfSquares < 0) { NewRelic.incrementCounter("Supportability/ResponseTimeStatsImpl/NegativeValue"); Agent.LOG.log(Level.INFO, "Invalid count {0}, total {1}, totalExclusive {2}, or sum of squares {3}", - count, total, totalExclusive, sumOfSquares); + getCallCount(), total, totalExclusive, sumOfSquares); } } } @@ -96,7 +96,7 @@ public void recordResponseTimeInNanos(long responseTime, long exclusiveTime) { public boolean hasData() { boolean hasData; synchronized (lock) { - hasData = count > 0 || total > 0 || totalExclusive > 0; + hasData = getCallCount() > 0 || total > 0 || totalExclusive > 0; } return hasData; } @@ -104,7 +104,7 @@ public boolean hasData() { @Override public void reset() { synchronized (lock) { - count = 0; + setCallCount(0); total = totalExclusive = minValue = maxValue = 0; sumOfSquares = 0; } @@ -151,14 +151,14 @@ public final void merge(StatsBase statsObj) { if (statsObj instanceof ResponseTimeStatsImpl) { ResponseTimeStatsImpl stats = (ResponseTimeStatsImpl) statsObj; synchronized (lock) { - if (stats.count > 0) { - if (count > 0) { + if (stats.getCallCount() > 0) { + if (getCallCount() > 0) { minValue = Math.min(minValue, stats.minValue); } else { minValue = stats.minValue; } } - count += stats.count; + incrementCallCount(stats.getCallCount()); total += stats.total; totalExclusive += stats.totalExclusive; @@ -172,7 +172,7 @@ public final void merge(StatsBase statsObj) { public void recordResponseTime(int count, long totalTime, long minTime, long maxTime, TimeUnit unit) { synchronized (lock) { long totalTimeInNanos = TimeUnit.NANOSECONDS.convert(totalTime, unit); - this.count = count; + this.setCallCount(count); this.total = totalTimeInNanos; this.totalExclusive = totalTimeInNanos; this.minValue = TimeUnit.NANOSECONDS.convert(minTime, unit); diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java index f036a4dd31..69675163cb 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java @@ -42,7 +42,7 @@ public StatsImpl(int count, float total, float minValue, float maxValue, double public Object clone() throws CloneNotSupportedException { StatsImpl newStats = new StatsImpl(); synchronized (lock) { - newStats.count = count; + newStats.setCallCount(this.getCallCount()); newStats.total = total; newStats.minValue = minValue; newStats.maxValue = maxValue; @@ -53,7 +53,7 @@ public Object clone() throws CloneNotSupportedException { @Override public String toString() { - return super.toString() + " [total=" + total + ", count=" + count + ", minValue=" + return super.toString() + " [total=" + total + ", count=" + this.getCallCount() + ", minValue=" + minValue + ", maxValue=" + maxValue + ", sumOfSquares=" + sumOfSquares + "]"; } @@ -67,20 +67,20 @@ public void recordDataPoint(float value) { if (sos < sumOfSquares) { throw new IllegalArgumentException("Data value " + value + " caused sum of squares to roll over"); } - if (count > 0) { + if (this.getCallCount() > 0) { minValue = Math.min(value, minValue); } else { minValue = value; } - count++; + incrementCallCount(); total += value; maxValue = Math.max(value, maxValue); sumOfSquares = sos; if (NewRelic.getAgent().getConfig().getValue(AgentConfigImpl.METRIC_DEBUG, AgentConfigImpl.DEFAULT_METRIC_DEBUG)) { - if (count < 0 || total < 0) { + if (getCallCount() < 0 || total < 0) { NewRelic.incrementCounter("Supportability/StatsImpl/NegativeValue"); - Agent.LOG.log(Level.INFO, "Invalid count {0} or total {1}", count, total); + Agent.LOG.log(Level.INFO, "Invalid count {0} or total {1}", getCallCount(), total); } } @@ -90,14 +90,14 @@ public void recordDataPoint(float value) { @Override public boolean hasData() { synchronized (lock) { - return count > 0 || total > 0; + return getCallCount() > 0 || total > 0; } } @Override public void reset() { synchronized (lock) { - count = 0; + setCallCount(0); total = minValue = maxValue = 0; sumOfSquares = 0; } @@ -143,14 +143,14 @@ public void merge(StatsBase statsObj) { if (statsObj instanceof StatsImpl) { StatsImpl stats = (StatsImpl) statsObj; synchronized (lock) { - if (stats.count > 0) { - if (count > 0) { + if (stats.getCallCount() > 0) { + if (getCallCount() > 0) { minValue = Math.min(minValue, stats.minValue); } else { minValue = stats.minValue; } } - count += stats.count; + setCallCount(stats.getCallCount()); total += stats.total; maxValue = Math.max(maxValue, stats.maxValue); From 6c3cba46ade43a21b7b26d75fcfe59b940844325 Mon Sep 17 00:00:00 2001 From: Todd W Crone Date: Thu, 3 Mar 2022 15:38:41 -0500 Subject: [PATCH 4/8] Add -2 to SNAPSHOT version to distinguish --- gradle.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle.properties b/gradle.properties index cb36acf60e..0863fdac34 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ # The agent version. -agentVersion=7.6.0-13353 +agentVersion=7.6.0-13353-2 newrelicDebug=false org.gradle.jvmargs=-Xmx2048m From 44cbcc1d93325e5563a0df86fc1313a10c3622d0 Mon Sep 17 00:00:00 2001 From: Todd W Crone Date: Thu, 3 Mar 2022 16:38:18 -0500 Subject: [PATCH 5/8] Add thread safety to merge() method --- .../com/newrelic/agent/stats/StatsImpl.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java index 69675163cb..82ca6ed649 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java @@ -35,20 +35,11 @@ public StatsImpl(int count, float total, float minValue, float maxValue, double this.minValue = minValue; this.maxValue = maxValue; this.sumOfSquares = sumOfSquares; - } @Override public Object clone() throws CloneNotSupportedException { - StatsImpl newStats = new StatsImpl(); - synchronized (lock) { - newStats.setCallCount(this.getCallCount()); - newStats.total = total; - newStats.minValue = minValue; - newStats.maxValue = maxValue; - newStats.sumOfSquares = sumOfSquares; - } - return newStats; + return this.copy(); } @Override @@ -138,10 +129,23 @@ public double getSumOfSquares() { } } + // make a local copy for merge() + private StatsImpl copy() { + StatsImpl newStats = new StatsImpl(); + synchronized (lock) { + newStats.setCallCount(this.getCallCount()); + newStats.total = total; + newStats.minValue = minValue; + newStats.maxValue = maxValue; + newStats.sumOfSquares = sumOfSquares; + } + return newStats; + } + @Override public void merge(StatsBase statsObj) { if (statsObj instanceof StatsImpl) { - StatsImpl stats = (StatsImpl) statsObj; + StatsImpl stats = ((StatsImpl) statsObj).copy(); synchronized (lock) { if (stats.getCallCount() > 0) { if (getCallCount() > 0) { From d8a65e0aedcace4724471ae7ad2bbce95b4d89d1 Mon Sep 17 00:00:00 2001 From: Todd W Crone Date: Thu, 3 Mar 2022 16:41:12 -0500 Subject: [PATCH 6/8] Add thread safety to merge() method --- .../agent/stats/ResponseTimeStatsImpl.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java index 813077ae19..f9658e9977 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/stats/ResponseTimeStatsImpl.java @@ -37,16 +37,7 @@ protected ResponseTimeStatsImpl() { @Override public Object clone() throws CloneNotSupportedException { - ResponseTimeStatsImpl newStats = new ResponseTimeStatsImpl(); - synchronized (lock) { - newStats.setCallCount(this.getCallCount()); - newStats.total = total; - newStats.totalExclusive = totalExclusive; - newStats.minValue = minValue; - newStats.maxValue = maxValue; - newStats.sumOfSquares = sumOfSquares; - } - return newStats; + return copy(); } @Override @@ -146,10 +137,23 @@ public double getSumOfSquares() { } } + private ResponseTimeStatsImpl copy() { + ResponseTimeStatsImpl newStats = new ResponseTimeStatsImpl(); + synchronized (lock) { + newStats.setCallCount(this.getCallCount()); + newStats.total = total; + newStats.totalExclusive = totalExclusive; + newStats.minValue = minValue; + newStats.maxValue = maxValue; + newStats.sumOfSquares = sumOfSquares; + } + return newStats; + } + @Override public final void merge(StatsBase statsObj) { if (statsObj instanceof ResponseTimeStatsImpl) { - ResponseTimeStatsImpl stats = (ResponseTimeStatsImpl) statsObj; + ResponseTimeStatsImpl stats = ((ResponseTimeStatsImpl) statsObj).copy(); synchronized (lock) { if (stats.getCallCount() > 0) { if (getCallCount() > 0) { From db6f65b0f34d591249399e9af04097817e1593dc Mon Sep 17 00:00:00 2001 From: Todd W Crone Date: Mon, 7 Mar 2022 13:10:38 -0500 Subject: [PATCH 7/8] Not sure why they needed to set via reflection --- .../newrelic/agent/stats/MetricDataTest.java | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/newrelic-agent/src/test/java/com/newrelic/agent/stats/MetricDataTest.java b/newrelic-agent/src/test/java/com/newrelic/agent/stats/MetricDataTest.java index eb4c8cf8ad..fdf603f4d1 100644 --- a/newrelic-agent/src/test/java/com/newrelic/agent/stats/MetricDataTest.java +++ b/newrelic-agent/src/test/java/com/newrelic/agent/stats/MetricDataTest.java @@ -7,20 +7,18 @@ package com.newrelic.agent.stats; -import java.io.ByteArrayOutputStream; -import java.io.IOException; -import java.io.PrintWriter; -import java.lang.reflect.Field; -import java.util.Arrays; - -import org.json.simple.JSONArray; -import org.junit.Assert; -import org.junit.Test; - import com.newrelic.agent.MetricData; import com.newrelic.agent.MetricNames; import com.newrelic.agent.metric.MetricName; import com.newrelic.agent.transport.DataSenderWriter; +import org.json.simple.JSONArray; +import org.junit.Assert; +import org.junit.Test; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.PrintWriter; +import java.util.Arrays; public class MetricDataTest { @@ -49,12 +47,9 @@ public void bad() throws IOException { } @Test - public void invalidStat() throws IllegalArgumentException, SecurityException, IllegalAccessException, - NoSuchFieldException { + public void invalidStat() throws IllegalArgumentException, SecurityException { StatsImpl stats = new StatsImpl(); - Field countField = AbstractStats.class.getDeclaredField("count"); - countField.setAccessible(true); - countField.set(stats, -1); + stats.setCallCount(-1); MetricData data = MetricData.create(MetricName.create("dude"), stats); String json = DataSenderWriter.toJSONString(data); Assert.assertEquals("[{\"name\":\"dude\"},[0,0,0,0,0,0]]", json); From a2a17d691d273f09893f40266b3ad86aef15bd33 Mon Sep 17 00:00:00 2001 From: Todd W Crone Date: Mon, 7 Mar 2022 13:29:17 -0500 Subject: [PATCH 8/8] Fix minor addition bug --- .../src/main/java/com/newrelic/agent/stats/StatsImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java b/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java index 82ca6ed649..8f93ba5842 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/stats/StatsImpl.java @@ -154,7 +154,7 @@ public void merge(StatsBase statsObj) { minValue = stats.minValue; } } - setCallCount(stats.getCallCount()); + incrementCallCount(stats.getCallCount()); total += stats.total; maxValue = Math.max(maxValue, stats.maxValue);