diff --git a/gradle.properties b/gradle.properties index 733be2d3de..0863fdac34 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,5 +1,5 @@ # The agent version. -agentVersion=7.6.0 +agentVersion=7.6.0-13353-2 newrelicDebug=false org.gradle.jvmargs=-Xmx2048m 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 903b631c55..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 @@ -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; @@ -35,14 +37,7 @@ 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; - return newStats; + return copy(); } @Override @@ -67,94 +62,129 @@ 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 (getCallCount() > 0) { + minValue = Math.min(responseTime, minValue); + } else { + minValue = responseTime; + } + incrementCallCount(); + total += responseTime; + maxValue = Math.max(responseTime, maxValue); + totalExclusive += exclusiveTime; + if (NewRelic.getAgent().getConfig().getValue(AgentConfigImpl.METRIC_DEBUG, AgentConfigImpl.DEFAULT_METRIC_DEBUG)) { + 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}", + getCallCount(), total, totalExclusive, sumOfSquares); + } } } - } @Override public boolean hasData() { - return count > 0 || total > 0 || totalExclusive > 0; + boolean hasData; + synchronized (lock) { + hasData = getCallCount() > 0 || total > 0 || totalExclusive > 0; + } + return hasData; } @Override public void reset() { - count = 0; - total = totalExclusive = minValue = maxValue = 0; - sumOfSquares = 0; + synchronized (lock) { + setCallCount(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; + } + } + + 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; - if (stats.count > 0) { - if (count > 0) { - minValue = Math.min(minValue, stats.minValue); - } else { - minValue = stats.minValue; + ResponseTimeStatsImpl stats = ((ResponseTimeStatsImpl) statsObj).copy(); + synchronized (lock) { + if (stats.getCallCount() > 0) { + if (getCallCount() > 0) { + minValue = Math.min(minValue, stats.minValue); + } else { + minValue = stats.minValue; + } } - } - count += stats.count; - total += stats.total; - totalExclusive += stats.totalExclusive; + incrementCallCount(stats.getCallCount()); + 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.setCallCount(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..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 @@ -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; @@ -33,23 +35,16 @@ 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(); - newStats.count = count; - newStats.total = total; - newStats.minValue = minValue; - newStats.maxValue = maxValue; - newStats.sumOfSquares = sumOfSquares; - return newStats; + return this.copy(); } @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 + "]"; } @@ -58,84 +53,113 @@ 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 (this.getCallCount() > 0) { + minValue = Math.min(value, minValue); + } else { + minValue = value; + } + 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) { - 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 (getCallCount() < 0 || total < 0) { + NewRelic.incrementCounter("Supportability/StatsImpl/NegativeValue"); + Agent.LOG.log(Level.INFO, "Invalid count {0} or total {1}", getCallCount(), total); + } } } - } @Override public boolean hasData() { - return count > 0 || total > 0; + synchronized (lock) { + return getCallCount() > 0 || total > 0; + } } @Override public void reset() { - count = 0; - total = minValue = maxValue = 0; - sumOfSquares = 0; + synchronized (lock) { + setCallCount(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; + } + } + + // 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; - if (stats.count > 0) { - if (count > 0) { - minValue = Math.min(minValue, stats.minValue); - } else { - minValue = stats.minValue; + StatsImpl stats = ((StatsImpl) statsObj).copy(); + synchronized (lock) { + if (stats.getCallCount() > 0) { + if (getCallCount() > 0) { + minValue = Math.min(minValue, stats.minValue); + } else { + minValue = stats.minValue; + } } - } - count += stats.count; - total += stats.total; + incrementCallCount(stats.getCallCount()); + total += stats.total; - maxValue = Math.max(maxValue, stats.maxValue); - sumOfSquares += stats.sumOfSquares; + maxValue = Math.max(maxValue, stats.maxValue); + sumOfSquares += stats.sumOfSquares; + } } } - } 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);