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

Convert client_java to OpenMetrics #615

Merged
merged 11 commits into from
Jan 25, 2021
50 changes: 43 additions & 7 deletions simpleclient/src/main/java/io/prometheus/client/Collector.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@

package io.prometheus.client;

import java.util.ArrayList;
import java.util.List;
import java.util.regex.Pattern;

Expand All @@ -19,27 +20,58 @@ public abstract class Collector {
*/
public abstract List<MetricFamilySamples> collect();
public enum Type {
UNKNOWN, // This is untyped in Prometheus text format.
COUNTER,
GAUGE,
SUMMARY,
STATE_SET,
INFO,
HISTOGRAM,
UNTYPED,
GAUGE_HISTOGRAM,
SUMMARY,
}

/**
* A metric, and all of its samples.
*/
static public class MetricFamilySamples {
public final String name;
public final String unit;
public final Type type;
public final String help;
public final List<Sample> samples;

public MetricFamilySamples(String name, Type type, String help, List<Sample> samples) {
public MetricFamilySamples(String name, String unit, Type type, String help, List<Sample> samples) {
if (!unit.isEmpty() && !name.endsWith("_" + unit)) {
throw new IllegalArgumentException("Metric's unit is not the suffix of the metric name: " + name);
}
if ((type == Type.INFO || type == Type.STATE_SET) && !unit.isEmpty()) {
throw new IllegalArgumentException("Metric is of a type that cannot have a unit: " + name);
}
List<Sample> mungedSamples = samples;
// Deal with _total from pre-OM automatically.
if (type == Type.COUNTER) {
if (name.endsWith("_total")) {

Choose a reason for hiding this comment

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

nit: Worth making _total a const? Then can name.endsWith(counterSuffix) and name = name.substring(0, name.length() - counterSuffix.length()).

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 don't like indirection like that, it only makes the code harder to understand for me as I have to bounce around the file for something that could be understood one its own line.

name = name.substring(0, name.length() - 6);
}
String withTotal = name + "_total";

Choose a reason for hiding this comment

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

nit: This could also use the const.

mungedSamples = new ArrayList<Sample>(samples.size());
for (Sample s: samples) {
String n = s.name;
if (name.equals(n)) {
n = withTotal;
}
mungedSamples.add(new Sample(n, s.labelNames, s.labelValues, s.value, s.timestampMs));
}
}
this.name = name;
this.unit = unit;
this.type = type;
this.help = help;
this.samples = samples;
this.samples = mungedSamples;
}

public MetricFamilySamples(String name, Type type, String help, List<Sample> samples) {
this(name, "", type, help, samples);
}

@Override
Expand All @@ -49,14 +81,18 @@ public boolean equals(Object obj) {
}
MetricFamilySamples other = (MetricFamilySamples) obj;

return other.name.equals(name) && other.type.equals(type)
&& other.help.equals(help) && other.samples.equals(samples) ;
return other.name.equals(name)
&& other.unit.equals(unit)
&& other.type.equals(type)
&& other.help.equals(help)
&& other.samples.equals(samples);
}

@Override
public int hashCode() {
int hash = 1;
hash = 37 * hash + name.hashCode();
hash = 37 * hash + unit.hashCode();
hash = 37 * hash + type.hashCode();
hash = 37 * hash + help.hashCode();
hash = 37 * hash + samples.hashCode();
Expand All @@ -65,7 +101,7 @@ public int hashCode() {

@Override
public String toString() {
return "Name: " + name + " Type: " + type + " Help: " + help +
return "Name: " + name + " Unit:" + unit + " Type: " + type + " Help: " + help +
" Samples: " + samples;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,32 @@ private List<String> collectorNames(Collector m) {
List<String> names = new ArrayList<String>();
for (Collector.MetricFamilySamples family : mfs) {
switch (family.type) {
case COUNTER:
names.add(family.name + "_total");
names.add(family.name + "_created");
names.add(family.name);
break;
case SUMMARY:
names.add(family.name + "_count");
names.add(family.name + "_sum");
names.add(family.name + "_created");
names.add(family.name);
break;
case HISTOGRAM:
names.add(family.name + "_count");
names.add(family.name + "_sum");
names.add(family.name + "_bucket");
names.add(family.name + "_created");
names.add(family.name);
break;
case GAUGE_HISTOGRAM:

Choose a reason for hiding this comment

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

Believe gauge histogram should also have _created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, gauges don't have a created.

Choose a reason for hiding this comment

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

If so, should we change the proto then? HistogramValue can be used with GAUGE_HISTOGRAM:
https://github.com/OpenObservability/OpenMetrics/blob/master/proto/openmetrics_data_model.proto#L136-L138

I suppose implementors could enforce that's not combined together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should clarify that then.

names.add(family.name + "_gcount");
names.add(family.name + "_gsum");
names.add(family.name + "_bucket");
names.add(family.name);
break;
case INFO:
names.add(family.name + "_info");
names.add(family.name);
break;
default:
Expand Down
20 changes: 19 additions & 1 deletion simpleclient/src/main/java/io/prometheus/client/Counter.java
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@
* </pre>
* These can be aggregated and processed together much more easily in the Prometheus
* server than individual metrics for each labelset.
*
* If there is a suffix of <code>_total</code> on the metric name, it will be
* removed. When exposing the time series for counter value, a
* <code>_total</code> suffix will be added. This is for compatibility between
* OpenMetrics and the Prometheus text format, as OpenMetrics requires the
* <code>_total</code> suffix.
*/
public class Counter extends SimpleCollector<Counter.Child> implements Collector.Describable {

Expand All @@ -74,6 +80,10 @@ public class Counter extends SimpleCollector<Counter.Child> implements Collector
public static class Builder extends SimpleCollector.Builder<Builder, Counter> {
@Override
public Counter create() {
// Gracefully handle pre-OpenMetrics counters.
if (name.endsWith("_total")) {
name = name.substring(0, name.length() - 6);
Comment on lines +84 to +85

Choose a reason for hiding this comment

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

nit: These could reference the _total const?

}
return new Counter(this);
}
}
Expand Down Expand Up @@ -108,6 +118,7 @@ protected Child newChild() {
*/
public static class Child {
private final DoubleAdder value = new DoubleAdder();
private final long created = System.currentTimeMillis();
/**
* Increment the counter by 1.
*/
Expand All @@ -130,6 +141,12 @@ public void inc(double amt) {
public double get() {
return value.sum();
}
/**
* Get the created time of the counter in milliseconds.
*/
public long created() {

Choose a reason for hiding this comment

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

Should this maybe be a float in seconds? That way callers don't need to divide by 1000.0 to get the value they should expose, also since this accessor doesn't have the unit (seconds/milliseconds/etc) maybe it's best to return the unit that's expected to be exposed for exposition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Java's current time function returns milliseconds, so that's the effective standard inside Java - and thus also what the client uses everywhere already.

return created;
}
}

// Convenience methods.
Expand Down Expand Up @@ -158,7 +175,8 @@ public double get() {
public List<MetricFamilySamples> collect() {
List<MetricFamilySamples.Sample> samples = new ArrayList<MetricFamilySamples.Sample>(children.size());
for(Map.Entry<List<String>, Child> c: children.entrySet()) {
samples.add(new MetricFamilySamples.Sample(fullname, labelNames, c.getKey(), c.getValue().get()));
samples.add(new MetricFamilySamples.Sample(fullname + "_total", labelNames, c.getKey(), c.getValue().get()));
samples.add(new MetricFamilySamples.Sample(fullname + "_created", labelNames, c.getKey(), c.getValue().created() / 1000.0));
}
return familySamplesList(Type.COUNTER, samples);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public CounterMetricFamily(String name, String help, double value) {
labelNames = Collections.emptyList();
samples.add(
new Sample(
name,
this.name + "_total",
labelNames,
Collections.<String>emptyList(),
value));
Expand All @@ -52,7 +52,7 @@ public CounterMetricFamily addMetric(List<String> labelValues, double value) {
if (labelValues.size() != labelNames.size()) {
throw new IllegalArgumentException("Incorrect number of labels.");
}
samples.add(new Sample(name, labelNames, labelValues, value));
samples.add(new Sample(name + "_total", labelNames, labelValues, value));
return this;
}
}