Skip to content

Split a metric when there're more than 100 data points #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Split a metric when there're more than 100 data points #74

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4df6ad4
Split a metric when there're more than 100 data points
yaozhaoy Apr 21, 2021
59e6992
Address comments
yaozhaoy Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/main/java/software/amazon/cloudwatchlogs/emf/Constants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,6 @@ public class Constants {
public static final String UNKNOWN = "Unknown";

public static final int MAX_METRICS_PER_EVENT = 100;

public static final int MAX_DATAPOINTS_PER_METRIC = 100;
}
62 changes: 49 additions & 13 deletions src/main/java/software/amazon/cloudwatchlogs/emf/model/MetricsContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,36 +203,72 @@ public MetricsContext createCopyWithContext() {
* metrics in one log event. If there're more than 100 metrics, we split the metrics into
* multiple log events.
*
* <p>If a metric has more than 100 data points, we also split the metric.
*
* @return the serialized strings.
* @throws JsonProcessingException if there's any object that cannot be serialized
*/
public List<String> serialize() throws JsonProcessingException {
if (rootNode.metrics().size() <= Constants.MAX_METRICS_PER_EVENT) {
if (rootNode.metrics().size() <= Constants.MAX_METRICS_PER_EVENT
&& !anyMetricWithTooManyDataPoints(rootNode)) {
return Arrays.asList(this.rootNode.serialize());
} else {
List<RootNode> nodes = new ArrayList<>();
Map<String, MetricDefinition> metrics = new HashMap<>();
int count = 0;
for (MetricDefinition metric : rootNode.metrics().values()) {
metrics.put(metric.getName(), metric);
count++;
Queue<MetricDefinition> metricDefinitions =
new LinkedList<>(rootNode.metrics().values());
Copy link
Member

@jaredcnance jaredcnance Apr 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. My personal preference here is to not use the abstract interface for your local variable. The concrete type (LinkedList) won't be changing and there's not much of a need to limit yourself to only the interface. This is just a style comment not a perf comment (dynamic dispatch won't be necessary in this context) so YMMV.
  2. This constructor forces enumeration of the set and allocation of a new list. Is this really necessary? It shouldn't be a significant issue, just something to think about.

while (metricDefinitions.size() > 0) {
MetricDefinition metric = metricDefinitions.peek();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need peek() if we're going to poll() regardless? Can we just poll() here?


if (metrics.size() == Constants.MAX_METRICS_PER_EVENT
|| count == rootNode.metrics().size()) {
Metadata metadata = rootNode.getAws();
MetricDirective metricDirective = metadata.getCloudWatchMetrics().get(0);
Metadata clonedMetadata =
metadata.withCloudWatchMetrics(
Arrays.asList(metricDirective.withMetrics(metrics)));
nodes.add(rootNode.withAws(clonedMetadata));
|| metrics.containsKey(metric.getName())) {
nodes.add(buildRootNode(metrics));
metrics = new HashMap<>();
}
}

metric = metricDefinitions.poll();
if (metric.getValues().size() <= Constants.MAX_DATAPOINTS_PER_METRIC) {
metrics.put(metric.getName(), metric);
} else {
metrics.put(
metric.getName(),
new MetricDefinition(
metric.getName(),
metric.getUnit(),
metric.getValues()
.subList(0, Constants.MAX_DATAPOINTS_PER_METRIC)));
metricDefinitions.offer(
new MetricDefinition(
metric.getName(),
metric.getUnit(),
metric.getValues()
.subList(
Constants.MAX_DATAPOINTS_PER_METRIC,
metric.getValues().size())));
}
}
if (!metrics.isEmpty()) {
nodes.add(buildRootNode(metrics));
}
List<String> strings = new ArrayList<>();
for (RootNode node : nodes) {
strings.add(node.serialize());
}
return strings;
}
}

private RootNode buildRootNode(Map<String, MetricDefinition> metrics) {
Metadata metadata = rootNode.getAws();
MetricDirective metricDirective = metadata.getCloudWatchMetrics().get(0);
Metadata clonedMetadata =
metadata.withCloudWatchMetrics(Arrays.asList(metricDirective.withMetrics(metrics)));
return rootNode.withAws(clonedMetadata);
}

private boolean anyMetricWithTooManyDataPoints(RootNode node) {
return node.metrics().values().stream()
.anyMatch(
metric -> metric.getValues().size() > Constants.MAX_DATAPOINTS_PER_METRIC);
}
}
64 changes: 62 additions & 2 deletions src/test/java/software/amazon/cloudwatchlogs/emf/model/MetricsContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,17 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.json.JsonMapper;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import org.junit.Test;
import software.amazon.cloudwatchlogs.emf.Constants;

public class MetricsContextTest {

Expand Down Expand Up @@ -75,6 +78,59 @@ public void testSerializeMoreThen100Metrics() throws JsonProcessingException {
}
}

@Test
public void testSerializeAMetricWith101DataPoints() throws JsonProcessingException {
MetricsContext mc = new MetricsContext();
int dataPointCount = 101;
int expectedEventCount = 2;
String metricName = "metric";
for (int i = 0; i < dataPointCount; i++) {
mc.putMetric(metricName, i);
}

List<String> events = mc.serialize();
assertEquals(expectedEventCount, events.size());
List<MetricDefinition> allMetrics = new ArrayList<>();
for (String event : events) {
allMetrics.addAll(parseMetrics(event));
}
List<Double> expectedValues = new ArrayList<>();
for (int i = 0; i < Constants.MAX_DATAPOINTS_PER_METRIC; i++) {
expectedValues.add((double) i);
}
assertEquals(expectedValues, allMetrics.get(0).getValues());
assertTrue(allMetrics.get(1).getValues().equals(Arrays.asList(100.0)));
}

@Test
public void testSerializeMetricsWith101DataPoints() throws JsonProcessingException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main purpose of this test is just to verify that splitting doesn't impact other metric data points added to the context?

Copy link
Contributor Author

@yaozhaoy yaozhaoy Apr 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It also tests that the other metrics should also be included in the first event if there're less than 100 metrics for the first event.

MetricsContext mc = new MetricsContext();
int dataPointCount = 101;
int expectedEventCount = 2;
String metricName = "metric1";
for (int i = 0; i < dataPointCount; i++) {
mc.putMetric(metricName, i);
}
mc.putMetric("metric2", 2);

List<String> events = mc.serialize();
assertEquals(expectedEventCount, events.size());

List<MetricDefinition> metricsFromEvent1 = parseMetrics(events.get(0));
List<MetricDefinition> metricsFromEvent2 = parseMetrics(events.get(1));

assertEquals(2, metricsFromEvent1.size());
List<Double> expectedValues = new ArrayList<>();
for (int i = 0; i < Constants.MAX_DATAPOINTS_PER_METRIC; i++) {
expectedValues.add((double) i);
}
assertEquals(expectedValues, metricsFromEvent1.get(0).getValues());
assertEquals(Arrays.asList(2.0), metricsFromEvent1.get(1).getValues());

assertEquals(1, metricsFromEvent2.size());
assertEquals(Arrays.asList(100.0), metricsFromEvent2.get(0).getValues());
}

@Test
public void testSerializeZeroMetric() throws JsonProcessingException {
MetricsContext mc = new MetricsContext();
Expand Down Expand Up @@ -106,8 +162,12 @@ private ArrayList<MetricDefinition> parseMetrics(String event) throws JsonProces
for (Map<String, String> metric : metrics) {
String name = metric.get("Name");
Unit unit = Unit.fromValue(metric.get("Unit"));
double value = (double) metadata_map.get(name);
metricDefinitions.add(new MetricDefinition(name, unit, value));
Object value = metadata_map.get(name);
if (value instanceof ArrayList) {
metricDefinitions.add(new MetricDefinition(name, unit, (ArrayList) value));
} else {
metricDefinitions.add(new MetricDefinition(name, unit, (double) value));
}
}
return metricDefinitions;
}
Expand Down