Skip to content
Merged
Show file tree
Hide file tree
Changes from 34 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,68 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jmxscraper.assertions;

import static io.opentelemetry.contrib.jmxscraper.assertions.AttributeValueMatcher.ANY_VALUE_MATCHER;

import java.util.Objects;

/** Implements functionality of matching data point attributes. */
public class AttributeMatcher {
private final String name;
private final AttributeValueMatcher attributeValueMatcher;

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

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

public String getName() {
return name;
}
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 that it returns the name of the attribute


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

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

Choose a reason for hiding this comment

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

this breaks the usual equals/hashcode contract, which is generally not a good idea as it does not preserve the "logical equality". If I understand we have two use-cases:

  • store it in a map for lookup by attribute name.
  • check if attribute is matching or not.

For the lookup, we should store it in a map with a call to getName as key, and for the attribute matching it would be simpler to have a public boolean matches(String name, String value) and inline the AttributeValueMatcher which is almost only doing a String.equals(...).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe equals/hashcode contract is not broken here sice two equal objects will always have the same hashcode. However I agree it's a bit tricky use of equals/hashcode, mostly to get Set<> equality working out of the box to support verification if specific set of attributes is matched by set of matchers. I'll think about cleaner solution to avoid confusion.

Copy link
Contributor Author

@robsunday robsunday Dec 4, 2024

Choose a reason for hiding this comment

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

Refactoring is done. So now code is explicitly calling matchesValue method instead of using equals/hashcode tandem behind the scene


@Override
public String toString() {
return attributeValueMatcher.getValue() == null
? '{' + name + '}'
: '{' + name + '=' + attributeValueMatcher.getValue() + '}';
}
;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.contrib.jmxscraper.assertions;

import java.util.Objects;

class AttributeValueMatcher {
static final AttributeValueMatcher ANY_VALUE_MATCHER = new AttributeValueMatcher();

private final String value;

AttributeValueMatcher() {
this(null);
}

AttributeValueMatcher(String value) {
this.value = value;
}

String getValue() {
return value;
}

/**
* Match the value held by this and the other AttributeValueMatcher instances. Null value means
* "any value", that's why if any of the values is null they are considered as matching. If both
* values are not null then they must be equal.
*
* @param other the other matcher to compare value with
* @return true if values held by both matchers are matching, false otherwise.
*/
boolean matchValue(AttributeValueMatcher other) {
if ((value == null) || (other.value == null)) {
return true;
}
return Objects.equals(value, other.value);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* 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() {}

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);
}

public static AttributeMatcher attributeWithAnyValue(String name) {
return new AttributeMatcher(name);
}

public static Set<AttributeMatcher> attributeSet(AttributeMatcher... attributes) {
return Arrays.stream(attributes).collect(Collectors.toSet());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,31 +5,30 @@

package io.opentelemetry.contrib.jmxscraper.assertions;

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

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.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Consumer;
import java.util.stream.Collectors;
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,66 +234,48 @@ private void dataPointsCommonCheck(List<NumberDataPoint> dataPoints) {
}

/**
* Verifies that all data points have all the expected attributes
* Verifies that all data points have the same expected one attribute
*
* @param attributes expected attributes
* @param expectedAttribute expected attribute
* @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(Collections.singleton(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 provided attribute matcher set matches attributes of at least one metric
* data point. Attribute matcher set is considered matching data point attributes if each matcher
* matches exactly one data point attribute
*
* @param attributeSets sets of attributes as maps
* @param attributeSets 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>... attributeSets) {
return checkDataPoints(
dataPoints -> {
dataPointsCommonCheck(dataPoints);

boolean[] matchedSets = new boolean[attributeSets.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());

Set<AttributeMatcher> dataPointAttributes = toSet(dataPoint.getAttributesList());
Copy link
Contributor

Choose a reason for hiding this comment

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

as suggested in the comment in AttributeMatcher this relies on the equals/hashcode implementation here, keeping a map would allow to avoid relying on this. We could probably add an extra check in the toMap implementation to ensure that there aren't any duplicate entries if there is an error in the test code.

Also, here we are using the AttributeMatcher class to carry the attributes key/values which is definitely not a "matcher".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved

int matchCount = 0;
for (int i = 0; i < attributeSets.length; i++) {
if (mapEquals(map, attributeSets[i])) {
if (attributeSets[i].equals(dataPointAttributes)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

here the call to equals should be replaced by calling the AttributeMatcher.matches(String name, String value) method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not so simple, it is a comparison of collections.

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(attributeSets));
integers.assertEqual(info, matchCount, 1);
}

Expand All @@ -302,29 +289,9 @@ public final MetricAssert hasDataPointsAttributes(Map<String, String>... attribu
});
}

/**
* 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()) {
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);
}

private static Map<String, String> toMap(List<KeyValue> list) {
private static Set<AttributeMatcher> toSet(List<KeyValue> list) {
return list.stream()
.collect(Collectors.toMap(KeyValue::getKey, kv -> kv.getValue().getStringValue()));
.map(entry -> attribute(entry.getKey(), entry.getValue().getStringValue()))
.collect(Collectors.toSet());
}
}
Loading
Loading