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

jmx state metrics #12369

Merged
merged 24 commits into from
Oct 17, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
9272bfe
minor test refactor
SylvainJuge Sep 20, 2024
6f20784
enhance tests
SylvainJuge Sep 24, 2024
d1a195b
add more assertions in tests
SylvainJuge Sep 25, 2024
3f2f151
add state mapping + tests
SylvainJuge Sep 30, 2024
5973b01
impl + test
SylvainJuge Sep 30, 2024
4fd10aa
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Sep 30, 2024
6155206
spotless
SylvainJuge Sep 30, 2024
af37bb4
thou shall lint
SylvainJuge Oct 1, 2024
6551200
minor add @Nullable
SylvainJuge Oct 1, 2024
376acc5
fix state metric detection
SylvainJuge Oct 1, 2024
679c460
lint again
SylvainJuge Oct 1, 2024
e5ff7c5
implement new syntax
SylvainJuge Oct 8, 2024
8e73f10
cleanup
SylvainJuge Oct 8, 2024
e9e8e83
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Oct 8, 2024
550b003
spotless
SylvainJuge Oct 8, 2024
1bb4302
add documentation
SylvainJuge Oct 9, 2024
ebf58ff
minor code refactor to use lists instead of arrays
SylvainJuge Oct 9, 2024
6c10e6d
port-review change in docs
SylvainJuge Oct 10, 2024
652ab32
switch to _ + enhance doc for 2 state
SylvainJuge Oct 14, 2024
e24b9ae
Update instrumentation/jmx-metrics/library/src/main/java/io/opentelem…
SylvainJuge Oct 16, 2024
2698be3
Merge branch 'main' of github.com:open-telemetry/opentelemetry-java-i…
SylvainJuge Oct 17, 2024
e4b9fe5
restore '*' as wildcard + clarify quoting
SylvainJuge Oct 17, 2024
111c26b
Merge branch 'state-jmx-metrics' of github.com:SylvainJuge/openteleme…
SylvainJuge Oct 17, 2024
96bb6cc
restore '*' again
SylvainJuge Oct 17, 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
Expand Up @@ -151,7 +151,7 @@ AttributeInfo getAttributeInfo(MBeanServerConnection connection, ObjectName obje

// Verify correctness of configuration by attempting to extract the metric value.
// The value will be discarded, but its type will be checked.
Object sampleValue = extractAttributeValue(connection, objectName, logger);
Object sampleValue = getSampleValue(connection, objectName);

// Only numbers can be used to generate metric values
if (sampleValue instanceof Number) {
Expand Down Expand Up @@ -194,6 +194,11 @@ AttributeInfo getAttributeInfo(MBeanServerConnection connection, ObjectName obje
return null;
}

@Nullable
protected Object getSampleValue(MBeanServerConnection connection, ObjectName objectName) {
return extractAttributeValue(connection, objectName, logger);
}
Comment on lines +198 to +200
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] this allows to bypass the sample value check for the "artificial" attribute value that we need for the state metric.


/**
* Extracts the specified attribute value. In case the value is a CompositeData, drills down into
* it to find the correct singleton value (usually a Number or a String).
Expand All @@ -203,7 +208,7 @@ AttributeInfo getAttributeInfo(MBeanServerConnection connection, ObjectName obje
* pattern
* @param logger the logger to use, may be null. Typically we want to log any issues with the
* attributes during MBean discovery, but once the attribute is successfully detected and
* confirmed to be eligble for metric evaluation, any further attribute extraction
* confirmed to be eligible for metric evaluation, any further attribute extraction
* malfunctions will be silent to avoid flooding the log.
* @return the attribute value, if found, or {@literal null} if an error occurred
*/
Expand Down Expand Up @@ -253,7 +258,8 @@ private Object extractAttributeValue(MBeanServerConnection connection, ObjectNam
}

@Nullable
Number extractNumericalAttribute(MBeanServerConnection connection, ObjectName objectName) {
protected Number extractNumericalAttribute(
MBeanServerConnection connection, ObjectName objectName) {
Object value = extractAttributeValue(connection, objectName);
if (value instanceof Number) {
return (Number) value;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ public MetricAttribute(String name, MetricAttributeExtractor extractor) {
this.extractor = extractor;
}

public boolean isStateAttribute() {
return extractor == null;
}

public String getAttributeName() {
return name;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ public class MetricInfo {
public enum Type {
COUNTER,
UPDOWNCOUNTER,
GAUGE
GAUGE,
/** state metric captured as updowncounter */
STATE
}

// How to report the metric using OpenTelemetry API
Expand All @@ -44,21 +46,21 @@ public MetricInfo(
this.type = type == null ? Type.GAUGE : type;
}

String getMetricName() {
public String getMetricName() {
return metricName;
}

@Nullable
String getDescription() {
public String getDescription() {
return description;
}

@Nullable
String getUnit() {
public String getUnit() {
return unit;
}

Type getType() {
public Type getType() {
return type;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,13 @@ void enrollExtractor(
}
logger.log(INFO, "Created Gauge for {0}", metricName);
}
break;
// CHECKSTYLE:OFF
case STATE:
{
// CHECKSTYLE:ON
throw new IllegalStateException("state metrics should not be registered");
}
Comment on lines +125 to +129
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] this is needed to have the switch statement cover all cases.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import io.opentelemetry.instrumentation.jmx.engine.BeanAttributeExtractor;
import io.opentelemetry.instrumentation.jmx.engine.BeanGroup;
import io.opentelemetry.instrumentation.jmx.engine.MetricAttribute;
import io.opentelemetry.instrumentation.jmx.engine.MetricAttributeExtractor;
import io.opentelemetry.instrumentation.jmx.engine.MetricDef;
import io.opentelemetry.instrumentation.jmx.engine.MetricExtractor;
import io.opentelemetry.instrumentation.jmx.engine.MetricInfo;
Expand All @@ -18,6 +19,7 @@
import java.util.Map;
import java.util.Set;
import javax.annotation.Nullable;
import javax.management.MBeanServerConnection;
import javax.management.MalformedObjectNameException;
import javax.management.ObjectName;

Expand Down Expand Up @@ -143,8 +145,7 @@ public MetricDef buildMetricDef() throws Exception {
}

Set<String> attrNames = mapping.keySet();
MetricExtractor[] metricExtractors = new MetricExtractor[attrNames.size()];
int n = 0;
List<MetricExtractor> metricExtractors = new ArrayList<>();
for (String attributeName : attrNames) {
BeanAttributeExtractor attrExtractor = BeanAttributeExtractor.fromName(attributeName);
// This is essentially the same as 'attributeName' but with escape characters removed
Expand All @@ -162,44 +163,112 @@ public MetricDef buildMetricDef() throws Exception {
metricInfo = m.buildMetricInfo(prefix, niceAttributeName, getUnit(), getMetricType());
}

List<MetricAttribute> attributeList;
List<MetricAttribute> ownAttributes = getAttributeList();
if (ownAttributes != null && m != null && m.getAttributeList() != null) {
// MetricAttributes have been specified at two levels, need to combine them
attributeList = combineMetricAttributes(ownAttributes, m.getAttributeList());
} else if (ownAttributes != null) {
attributeList = ownAttributes;
} else if (m != null && m.getAttributeList() != null) {
// Get the attributes from the metric
attributeList = m.getAttributeList();
List<MetricAttribute> metricAttributes = m != null ? m.getAttributeList() : null;
List<MetricAttribute> attributeList =
combineMetricAttributes(ownAttributes, metricAttributes);

// higher priority to metric level mapping, then jmx rule as fallback
StateMapping stateMapping = getEffectiveStateMapping(m, this);

if (stateMapping.isEmpty()) {
metricExtractors.add(
new MetricExtractor(
attrExtractor, metricInfo, attributeList.toArray(new MetricAttribute[0])));
} else {
// There are no attributes at all
attributeList = new ArrayList<MetricAttribute>();

// generate one metric extractor per state metric key
// each metric extractor will have the state attribute replaced with a constant
metricExtractors.addAll(
createStateMappingExtractors(stateMapping, attributeList, attrExtractor, metricInfo));
}
}

return new MetricDef(group, metricExtractors.toArray(new MetricExtractor[0]));
}

private static List<MetricExtractor> createStateMappingExtractors(
StateMapping stateMapping,
List<MetricAttribute> attributeList,
BeanAttributeExtractor attrExtractor,
MetricInfo metricInfo) {

List<MetricExtractor> extractors = new ArrayList<>();
for (String key : stateMapping.getStateKeys()) {
List<MetricAttribute> stateMetricAttributes = new ArrayList<>();

for (MetricAttribute ma : attributeList) {
// replace state metric attribute with constant key value
if (!ma.isStateAttribute()) {
stateMetricAttributes.add(ma);
} else {
stateMetricAttributes.add(
new MetricAttribute(
ma.getAttributeName(), MetricAttributeExtractor.fromConstant(key)));
}
}

MetricExtractor metricExtractor =
BeanAttributeExtractor stateMetricExtractor =
new BeanAttributeExtractor(attrExtractor.getAttributeName()) {

@Override
protected Object getSampleValue(
MBeanServerConnection connection, ObjectName objectName) {
// metric actual type is sampled in the discovery process, so we have to
// make this extractor as extracting integers.
return 0;
}

@Override
protected Number extractNumericalAttribute(
MBeanServerConnection connection, ObjectName objectName) {
String rawStateValue = attrExtractor.extractValue(connection, objectName);
String mappedStateValue = stateMapping.getStateValue(rawStateValue);
return key.equals(mappedStateValue) ? 1 : 0;
}
};

// state metric are always up/down counters
MetricInfo stateMetricInfo =
new MetricInfo(
metricInfo.getMetricName(),
metricInfo.getDescription(),
metricInfo.getUnit(),
MetricInfo.Type.UPDOWNCOUNTER);

extractors.add(
new MetricExtractor(
attrExtractor,
metricInfo,
attributeList.toArray(new MetricAttribute[attributeList.size()]));
metricExtractors[n++] = metricExtractor;
stateMetricExtractor,
stateMetricInfo,
stateMetricAttributes.toArray(new MetricAttribute[0])));
}

return new MetricDef(group, metricExtractors);
return extractors;
}

private static List<MetricAttribute> combineMetricAttributes(
List<MetricAttribute> ownAttributes, List<MetricAttribute> metricAttributes) {

Map<String, MetricAttribute> set = new HashMap<>();
for (MetricAttribute ownAttribute : ownAttributes) {
set.put(ownAttribute.getAttributeName(), ownAttribute);
if (ownAttributes != null) {
for (MetricAttribute ownAttribute : ownAttributes) {
set.put(ownAttribute.getAttributeName(), ownAttribute);
}
}

// Let the metric level defined attributes override own attributes
for (MetricAttribute metricAttribute : metricAttributes) {
set.put(metricAttribute.getAttributeName(), metricAttribute);
if (metricAttributes != null) {
// Let the metric level defined attributes override own attributes
for (MetricAttribute metricAttribute : metricAttributes) {
set.put(metricAttribute.getAttributeName(), metricAttribute);
}
}

return new ArrayList<MetricAttribute>(set.values());
return new ArrayList<>(set.values());
}

private static StateMapping getEffectiveStateMapping(Metric m, JmxRule rule) {
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] for state mapping, the resulting mapping is not a combination of rule level and metric level definitions. Only one of them is used.

if (m == null || m.getStateMapping().isEmpty()) {
return rule.getStateMapping();
} else {
return m.getStateMapping();
}
}
}
Loading
Loading