Skip to content
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.

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

Assertions refactoring #1580

Merged
merged 52 commits into from
Dec 6, 2024
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
c3deb51
New metrics validation framework fundamentals.
robsunday Nov 22, 2024
fcf2bf8
Spotless fixes
robsunday Nov 22, 2024
d833450
Improved check for not received metrics
robsunday Nov 25, 2024
8cb96ad
Cassandra integration test converted
robsunday Nov 25, 2024
f3bac7e
Fine tuning assertion messages.
robsunday Nov 25, 2024
c9eb0a7
ActiveMqIntegrationTest converted
robsunday Nov 25, 2024
2c85c38
Spotless fix
robsunday Nov 26, 2024
bd1947f
introduce 'register' API
SylvainJuge Nov 26, 2024
1e64f80
introduce dedicated assertThat for metrics
SylvainJuge Nov 26, 2024
f7c6373
refactor metrics verifier
SylvainJuge Nov 26, 2024
b10a340
add some javadoc & few comments
SylvainJuge Nov 26, 2024
65ddfb3
spotless & minor things
SylvainJuge Nov 26, 2024
db54835
add new assertion for attribute entries
SylvainJuge Nov 26, 2024
1bdf694
check for missing assertions
SylvainJuge Nov 26, 2024
9f8a394
verify attributes are checked in strict mode
SylvainJuge Nov 27, 2024
6fb7960
enhance datapoint attributes check
SylvainJuge Nov 27, 2024
a458c1c
comments, cleanup and inline a bit
SylvainJuge Nov 27, 2024
b9f054c
strict check avoids duplicate assertions
SylvainJuge Nov 28, 2024
cf72d19
remove obsolete comments in activemq yaml
SylvainJuge Nov 28, 2024
eab7c69
register -> add
SylvainJuge Nov 28, 2024
9c3390c
reformat
SylvainJuge Nov 28, 2024
9392780
refactor cassandra
SylvainJuge Nov 28, 2024
5ae735d
fix lint
SylvainJuge Nov 28, 2024
2c65318
refactor activemq
SylvainJuge Nov 28, 2024
a670561
refactor jvm metrics
SylvainJuge Nov 28, 2024
df09034
remove unused code
SylvainJuge Nov 28, 2024
06a968f
recycle assertions when we can
SylvainJuge Nov 28, 2024
0e5fd2c
Merge pull request #4 from SylvainJuge/assertions-refactoring-more
robsunday Nov 28, 2024
8062a96
Added some JavaDocs.
robsunday Nov 28, 2024
ba95efa
Merge branch 'main' into assertions-refactoring
robsunday Nov 29, 2024
4bed658
Cleanup
robsunday Nov 29, 2024
fddc5fc
Spotless fix
robsunday Nov 29, 2024
fd82042
Refactoring of data point attribute assertions
robsunday Dec 4, 2024
80994f2
Merge branch 'main' into assertions-refactoring
robsunday Dec 4, 2024
b26cf42
Coe review changes
robsunday Dec 4, 2024
0bf10f5
JavaDoc update
robsunday Dec 4, 2024
9f47ef6
JavaDoc update
robsunday Dec 4, 2024
2971ef7
Update jmx-scraper/src/integrationTest/java/io/opentelemetry/contrib/…
robsunday Dec 5, 2024
7d7e504
Code review changes
robsunday Dec 5, 2024
19f5d4c
Cleanup
robsunday Dec 5, 2024
6431feb
Additional comments added
robsunday Dec 5, 2024
ba0f900
add attribute matcher set class
SylvainJuge Dec 5, 2024
fdc64e3
remove equals/hashcode
SylvainJuge Dec 5, 2024
7754e3a
use AttributeMatcherSet for matching
SylvainJuge Dec 5, 2024
7b7f0d0
code cleanup
SylvainJuge Dec 5, 2024
cdf4c74
Merge branch 'assertions-refactoring' of github.com:robsunday/opentel…
SylvainJuge Dec 5, 2024
b170021
move set matching
SylvainJuge Dec 5, 2024
59a7dfc
inline conversion to map
SylvainJuge Dec 5, 2024
34b3ab3
reformat & cleanup
SylvainJuge Dec 5, 2024
45ba126
simplify matchesValue
SylvainJuge Dec 6, 2024
5e69a82
rename attribute set -> group
SylvainJuge Dec 6, 2024
dfe3af3
Merge pull request #6 from SylvainJuge/assertions-refactoring
robsunday Dec 6, 2024
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jmxscraper.assertions;

import java.util.Objects;

/** Implements functionality of matching data point attributes. */
public class AttributeMatcher {
private final String attributeName;
private final String attributeValue;
robsunday marked this conversation as resolved.
Show resolved Hide resolved

/**
* Create instance used to match data point attribute with any value.
*
* @param attributeName matched attribute name
*/
AttributeMatcher(String attributeName) {
this.attributeName = attributeName;
this.attributeValue = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Constructors should call each other:

Suggested change
this.attributeName = attributeName;
this.attributeValue = null;
this(attributeName, null);

}

/**
* Create instance used to match data point attribute with te same name and with the same value.
*
* @param attributeName attribute name
* @param attributeValue attribute value
*/
AttributeMatcher(String attributeName, String attributeValue) {
this.attributeName = attributeName;
this.attributeValue = attributeValue;
}

/**
* Return name of data point attribute that this AttributeMatcher is supposed to match value with.
*
* @return name of validated attribute
*/
public String getAttributeName() {
return attributeName;
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof AttributeMatcher)) {
return false;
}
AttributeMatcher other = (AttributeMatcher) o;
return Objects.equals(attributeName, other.attributeName);
}

@Override
public int hashCode() {
// Do not use value matcher here to support value wildcards
return Objects.hash(attributeName);
}

@Override
public String toString() {
return attributeValue == null
? '{' + attributeName + '}'
: '{' + attributeName + '=' + attributeValue + '}';
}

/**
* Verify if this matcher is matching provided attribute value. If this matcher holds null value
* then it is matching any attribute value
*
* @param value a value to be matched
* @return true if this matcher is matching provided value, false otherwise.
*/
boolean matchesValue(String value) {
if ((attributeValue == null) || (value == null)) {
return true;
}
return Objects.equals(attributeValue, value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm, this looks like this would be true:

new AttributeMatcher('foo', 'bar').matchesValue(null)

Is that right? Intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! This is leftover from the previous approach that I missed to update.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jmxscraper.assertions;

import java.util.Arrays;
import java.util.Set;
import java.util.stream.Collectors;

/**
* Utility class implementing convenience static methods to construct data point attribute matchers
* and sets of matchers.
*/
public class DataPointAttributes {
private DataPointAttributes() {}

/**
* Create instance of matcher that will be used to check if data point attribute with given name
* has provided value.
*
* @param name name of the data point attribute to check
* @param value expected value of data point attribute
* @return instance of matcher
*/
public static AttributeMatcher attribute(String name, String value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] add javadoc to explain the differences between all those methods: exact match, match with any value (even if the method name is quite clear on the latter, but not on the first on the exact match here).

return new AttributeMatcher(name, value);
}

/**
* Create instance of matcher that will be used to check if data point attribute with given name
* exists. Any value of the attribute is considered as matching.
*
* @param name name of the data point attribute to check
* @return instance of matcher
*/
public static AttributeMatcher attributeWithAnyValue(String name) {
return new AttributeMatcher(name);
}

/**
* Create a set of attribute matchers that will be used to verify set of data point attributes.
*
* @param attributes list of matchers to create set. It must contain matchers with unique names.
* @return set of unique attribute matchers
* @throws IllegalArgumentException if provided list contains two or more matchers with the same
* name.
* @see MetricAssert#hasDataPointsWithAttributes(Set[]) for detailed description of the algorithm
* of matching
*/
public static Set<AttributeMatcher> attributeSet(AttributeMatcher... attributes) {
Set<AttributeMatcher> matcherSet = Arrays.stream(attributes).collect(Collectors.toSet());
if (matcherSet.size() < attributes.length) {
throw new IllegalArgumentException(
"Duplicated matchers found in " + Arrays.toString(attributes));
}
return matcherSet;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@

package io.opentelemetry.contrib.jmxscraper.assertions;

import static io.opentelemetry.contrib.jmxscraper.assertions.DataPointAttributes.attributeSet;

import com.google.errorprone.annotations.CanIgnoreReturnValue;
import io.opentelemetry.proto.common.v1.KeyValue;
import io.opentelemetry.proto.metrics.v1.Metric;
import io.opentelemetry.proto.metrics.v1.NumberDataPoint;
import java.util.Arrays;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand All @@ -21,15 +22,13 @@
import org.assertj.core.api.AbstractAssert;
import org.assertj.core.internal.Integers;
import org.assertj.core.internal.Iterables;
import org.assertj.core.internal.Maps;
import org.assertj.core.internal.Objects;

public class MetricAssert extends AbstractAssert<MetricAssert, Metric> {

private static final Objects objects = Objects.instance();
private static final Iterables iterables = Iterables.instance();
private static final Integers integers = Integers.instance();
private static final Maps maps = Maps.instance();

private boolean strict;

Expand Down Expand Up @@ -59,7 +58,7 @@ private void strictCheck(
if (!strict) {
return;
}
String failMsgPrefix = expectedCheckStatus ? "duplicate" : "missing";
String failMsgPrefix = expectedCheckStatus ? "Missing" : "Duplicate";
info.description(
"%s assertion on %s for metric '%s'", failMsgPrefix, metricProperty, actual.getName());
objects.assertEqual(info, actualCheckStatus, expectedCheckStatus);
Expand Down Expand Up @@ -122,8 +121,8 @@ private MetricAssert hasSum(boolean monotonic) {
info.description("sum expected for metric '%s'", actual.getName());
objects.assertEqual(info, actual.hasSum(), true);

String prefix = monotonic ? "monotonic" : "non-monotonic";
info.description(prefix + " sum expected for metric '%s'", actual.getName());
String sumType = monotonic ? "monotonic" : "non-monotonic";
info.description("sum for metric '%s' is expected to be %s", actual.getName(), sumType);
objects.assertEqual(info, actual.getSum().getIsMonotonic(), monotonic);
return this;
}
Expand Down Expand Up @@ -156,6 +155,11 @@ public MetricAssert isUpDownCounter() {
return this;
}

/**
* Verifies that there is no attribute in any of data points.
*
* @return this
*/
@CanIgnoreReturnValue
public MetricAssert hasDataPointsWithoutAttributes() {
isNotNull();
Expand Down Expand Up @@ -195,6 +199,7 @@ private MetricAssert checkDataPoints(Consumer<List<NumberDataPoint>> listConsume
return this;
}

// TODO: To be removed and calls will be replaced with hasDataPointsWithAttributes()
@CanIgnoreReturnValue
public MetricAssert hasTypedDataPoints(Collection<String> types) {
return checkDataPoints(
Expand Down Expand Up @@ -229,98 +234,75 @@ private void dataPointsCommonCheck(List<NumberDataPoint> dataPoints) {
}

/**
* Verifies that all data points have all the expected attributes
* Verifies that all metric data points have the same expected one attribute
*
* @param attributes expected attributes
* @param expectedAttribute attribute matcher to validate data points attributes
* @return this
*/
@SafeVarargs
@CanIgnoreReturnValue
public final MetricAssert hasDataPointsAttributes(Map.Entry<String, String>... attributes) {
return checkDataPoints(
dataPoints -> {
dataPointsCommonCheck(dataPoints);

Map<String, String> attributesMap = new HashMap<>();
for (Map.Entry<String, String> attributeEntry : attributes) {
attributesMap.put(attributeEntry.getKey(), attributeEntry.getValue());
}
for (NumberDataPoint dataPoint : dataPoints) {
Map<String, String> dataPointAttributes = toMap(dataPoint.getAttributesList());

// all attributes must match
info.description(
"missing/unexpected data points attributes for metric '%s'", actual.getName());
containsExactly(dataPointAttributes, attributes);
maps.assertContainsAllEntriesOf(info, dataPointAttributes, attributesMap);
}
});
public final MetricAssert hasDataPointsWithOneAttribute(AttributeMatcher expectedAttribute) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[for reviewer] I left this method for convenience, because there are lots of assertions with just one attribute, but I can remove it if it makes matching logic harder to understand.

return hasDataPointsWithAttributes(attributeSet(expectedAttribute));
}

/**
* Verifies that all data points have their attributes match one of the attributes set and that
* all provided attributes sets matched at least once.
* Verifies that every data point attributes set is matched exactly by one of the matcher sets provided.
* Also, each matcher set must match at least one data point attributes set.
* Data point attributes set is matched by matcher set if each attribute is matched by one matcher and
* each matcher matches one attribute.
*
* @param attributeSets sets of attributes as maps
* @param attributeMatchers array of attribute matcher sets
* @return this
*/
@SafeVarargs
@CanIgnoreReturnValue
@SuppressWarnings("varargs") // required to avoid warning
public final MetricAssert hasDataPointsAttributes(Map<String, String>... attributeSets) {
public final MetricAssert hasDataPointsWithAttributes(
Set<AttributeMatcher>... attributeMatchers) {
return checkDataPoints(
dataPoints -> {
dataPointsCommonCheck(dataPoints);

boolean[] matchedSets = new boolean[attributeSets.length];
boolean[] matchedSets = new boolean[attributeMatchers.length];

// validate each datapoint attributes match exactly one of the provided attributes set
// validate each datapoint attributes match exactly one of the provided attributes sets
for (NumberDataPoint dataPoint : dataPoints) {
Map<String, String> map = toMap(dataPoint.getAttributesList());

Map<String, String> dataPointAttributes = toMap(dataPoint.getAttributesList());
int matchCount = 0;
for (int i = 0; i < attributeSets.length; i++) {
if (mapEquals(map, attributeSets[i])) {
for (int i = 0; i < attributeMatchers.length; i++) {
if (matchAttributes(attributeMatchers[i], dataPointAttributes)) {
matchedSets[i] = true;
matchCount++;
}
}

info.description(
"data point attributes '%s' for metric '%s' must match exactly one of the attribute sets '%s'",
map, actual.getName(), Arrays.asList(attributeSets));
dataPointAttributes, actual.getName(), Arrays.asList(attributeMatchers));
integers.assertEqual(info, matchCount, 1);
}

// check that all attribute sets matched at least once
for (int i = 0; i < matchedSets.length; i++) {
info.description(
"no data point matched attribute set '%s' for metric '%s'",
attributeSets[i], actual.getName());
attributeMatchers[i], actual.getName());
objects.assertEqual(info, matchedSets[i], true);
}
});
}

/**
* Map equality utility
*
* @param m1 first map
* @param m2 second map
* @return true if the maps have exactly the same keys and values
*/
private static boolean mapEquals(Map<String, String> m1, Map<String, String> m2) {
if (m1.size() != m2.size()) {
private static boolean matchAttributes(
Set<AttributeMatcher> attributeMatchers, Map<String, String> dataPointAttributes) {
if (attributeMatchers.size() != dataPointAttributes.size()) {
return false;
}
return m1.entrySet().stream().allMatch(e -> e.getValue().equals(m2.get(e.getKey())));
}

@SafeVarargs
@SuppressWarnings("varargs") // required to avoid warning
private final void containsExactly(
Map<String, String> map, Map.Entry<String, String>... entries) {
maps.assertContainsExactly(info, map, entries);
for (AttributeMatcher matcher : attributeMatchers) {
String attributeValue = dataPointAttributes.get(matcher.getAttributeName());
if (!matcher.matchesValue(attributeValue)) {
return false;
}
}
return true;
}

private static Map<String, String> toMap(List<KeyValue> list) {
Expand Down
Loading
Loading