Merge pull request #133 from prometheus/beorn7/improve-error-messages

Fix a number of minor things.

model/metric.go

@@ -121,7 +121,7 @@ func (m Metric) Fingerprint() Fingerprint {
 	return metricToFingerprint(m)
 }
 
-// Fingerprint returns a Metric's Fingerprint calculated by a faster hashing
+// FastFingerprint returns a Metric's Fingerprint calculated by a faster hashing
 // algorithm, which is, however, more susceptible to hash collisions.
 func (m Metric) FastFingerprint() Fingerprint {
 	return metricToFastFingerprint(m)

prometheus/registry.go

@@ -170,6 +170,11 @@ func Unregister(c Collector) bool {
 // checks are performed, but no further consistency checks (which would require
 // knowledge of a metric descriptor).
 //
+// Sorting concerns: The caller is responsible for sorting the label pairs in
+// each metric. However, the order of metrics will be sorted by the registry as
+// it is required anyway after merging with the metric families collected
+// conventionally.
+//
 // The function must be callable at any time and concurrently.
 func SetMetricFamilyInjectionHook(hook func() []*dto.MetricFamily) {
 	defRegistry.metricFamilyInjectionHook = hook
@@ -520,10 +525,11 @@ func (r *registry) checkConsistency(metricFamily *dto.MetricFamily, dtoMetric *d
 	if metricFamily.GetType() == dto.MetricType_GAUGE && dtoMetric.Gauge == nil ||
 		metricFamily.GetType() == dto.MetricType_COUNTER && dtoMetric.Counter == nil ||
 		metricFamily.GetType() == dto.MetricType_SUMMARY && dtoMetric.Summary == nil ||
+		metricFamily.GetType() == dto.MetricType_HISTOGRAM && dtoMetric.Histogram == nil ||
 		metricFamily.GetType() == dto.MetricType_UNTYPED && dtoMetric.Untyped == nil {
 		return fmt.Errorf(
-			"collected metric %q is not a %s",
+			"collected metric %s %s is not a %s",
-			dtoMetric, metricFamily.Type,
+			metricFamily.GetName(), dtoMetric, metricFamily.GetType(),
 		)
 	}
 
@@ -533,6 +539,11 @@ func (r *registry) checkConsistency(metricFamily *dto.MetricFamily, dtoMetric *d
 	buf.WriteString(metricFamily.GetName())
 	buf.WriteByte(model.SeparatorByte)
 	h.Write(buf.Bytes())
+	// Make sure label pairs are sorted. We depend on it for the consistency
+	// check. Label pairs must be sorted by contract. But the point of this
+	// method is to check for contract violations. So we better do the sort
+	// now.
+	sort.Sort(LabelPairSorter(dtoMetric.Label))
 	for _, lp := range dtoMetric.Label {
 		buf.Reset()
 		buf.WriteString(lp.GetValue())
@@ -542,8 +553,8 @@ func (r *registry) checkConsistency(metricFamily *dto.MetricFamily, dtoMetric *d
 	metricHash := h.Sum64()
 	if _, exists := metricHashes[metricHash]; exists {
 		return fmt.Errorf(
-			"collected metric %q was collected before with the same name and label values",
+			"collected metric %s %s was collected before with the same name and label values",
-			dtoMetric,
+			metricFamily.GetName(), dtoMetric,
 		)
 	}
 	metricHashes[metricHash] = struct{}{}
@@ -555,14 +566,14 @@ func (r *registry) checkConsistency(metricFamily *dto.MetricFamily, dtoMetric *d
 	// Desc consistency with metric family.
 	if metricFamily.GetName() != desc.fqName {
 		return fmt.Errorf(
-			"collected metric %q has name %q but should have %q",
+			"collected metric %s %s has name %q but should have %q",
-			dtoMetric, metricFamily.GetName(), desc.fqName,
+			metricFamily.GetName(), dtoMetric, metricFamily.GetName(), desc.fqName,
 		)
 	}
 	if metricFamily.GetHelp() != desc.help {
 		return fmt.Errorf(
-			"collected metric %q has help %q but should have %q",
+			"collected metric %s %s has help %q but should have %q",
-			dtoMetric, metricFamily.GetHelp(), desc.help,
+			metricFamily.GetName(), dtoMetric, metricFamily.GetHelp(), desc.help,
 		)
 	}
 
@@ -576,8 +587,8 @@ func (r *registry) checkConsistency(metricFamily *dto.MetricFamily, dtoMetric *d
 	}
 	if len(lpsFromDesc) != len(dtoMetric.Label) {
 		return fmt.Errorf(
-			"labels in collected metric %q are inconsistent with descriptor %s",
+			"labels in collected metric %s %s are inconsistent with descriptor %s",
-			dtoMetric, desc,
+			metricFamily.GetName(), dtoMetric, desc,
 		)
 	}
 	sort.Sort(LabelPairSorter(lpsFromDesc))
@@ -586,8 +597,8 @@ func (r *registry) checkConsistency(metricFamily *dto.MetricFamily, dtoMetric *d
 		if lpFromDesc.GetName() != lpFromMetric.GetName() ||
 			lpFromDesc.Value != nil && lpFromDesc.GetValue() != lpFromMetric.GetValue() {
 			return fmt.Errorf(
-				"labels in collected metric %q are inconsistent with descriptor %s",
+				"labels in collected metric %s %s are inconsistent with descriptor %s",
-				dtoMetric, desc,
+				metricFamily.GetName(), dtoMetric, desc,
 			)
 		}
 	}
@@ -597,7 +608,10 @@ func (r *registry) checkConsistency(metricFamily *dto.MetricFamily, dtoMetric *d
 
 	// Is the desc registered?
 	if _, exist := r.descIDs[desc.id]; !exist {
-		return fmt.Errorf("collected metric %q with unregistered descriptor %s", dtoMetric, desc)
+		return fmt.Errorf(
+			"collected metric %s %s with unregistered descriptor %s",
+			metricFamily.GetName(), dtoMetric, desc,
+		)
 	}
 
 	return nil

prometheus/registry_test.go

@@ -68,14 +68,14 @@ func testHandler(t testing.TB) {
 		Metric: []*dto.Metric{
 			{
 				Label: []*dto.LabelPair{
-					{
-						Name:  proto.String("externallabelname"),
-						Value: proto.String("externalval1"),
-					},
 					{
 						Name:  proto.String("externalconstname"),
 						Value: proto.String("externalconstvalue"),
 					},
+					{
+						Name:  proto.String("externallabelname"),
+						Value: proto.String("externalval1"),
+					},
 				},
 				Counter: &dto.Counter{
 					Value: proto.Float64(1),
@@ -100,27 +100,27 @@ func testHandler(t testing.TB) {
 	externalMetricFamilyAsBytes := externalBuf.Bytes()
 	externalMetricFamilyAsText := []byte(`# HELP externalname externaldocstring
 # TYPE externalname counter
-externalname{externallabelname="externalval1",externalconstname="externalconstvalue"} 1
+externalname{externalconstname="externalconstvalue",externallabelname="externalval1"} 1
 `)
 	externalMetricFamilyAsProtoText := []byte(`name: "externalname"
 help: "externaldocstring"
 type: COUNTER
 metric: <
-  label: <
-    name: "externallabelname"
-    value: "externalval1"
-  >
   label: <
     name: "externalconstname"
     value: "externalconstvalue"
   >
+  label: <
+    name: "externallabelname"
+    value: "externalval1"
+  >
   counter: <
     value: 1
   >
 >
 
 `)
-	externalMetricFamilyAsProtoCompactText := []byte(`name:"externalname" help:"externaldocstring" type:COUNTER metric:<label:<name:"externallabelname" value:"externalval1" > label:<name:"externalconstname" value:"externalconstvalue" > counter:<value:1 > > 
+	externalMetricFamilyAsProtoCompactText := []byte(`name:"externalname" help:"externaldocstring" type:COUNTER metric:<label:<name:"externalconstname" value:"externalconstvalue" > label:<name:"externallabelname" value:"externalval1" > counter:<value:1 > > 
 `)
 
 	expectedMetricFamily := &dto.MetricFamily{

text/create.go

@@ -79,7 +79,7 @@ func MetricFamilyToText(out io.Writer, in *dto.MetricFamily) (int, error) {
 		case dto.MetricType_COUNTER:
 			if metric.Counter == nil {
 				return written, fmt.Errorf(
-					"expected counter in metric %s", metric,
+					"expected counter in metric %s %s", name, metric,
 				)
 			}
 			n, err = writeSample(
@@ -90,7 +90,7 @@ func MetricFamilyToText(out io.Writer, in *dto.MetricFamily) (int, error) {
 		case dto.MetricType_GAUGE:
 			if metric.Gauge == nil {
 				return written, fmt.Errorf(
-					"expected gauge in metric %s", metric,
+					"expected gauge in metric %s %s", name, metric,
 				)
 			}
 			n, err = writeSample(
@@ -101,7 +101,7 @@ func MetricFamilyToText(out io.Writer, in *dto.MetricFamily) (int, error) {
 		case dto.MetricType_UNTYPED:
 			if metric.Untyped == nil {
 				return written, fmt.Errorf(
-					"expected untyped in metric %s", metric,
+					"expected untyped in metric %s %s", name, metric,
 				)
 			}
 			n, err = writeSample(
@@ -112,7 +112,7 @@ func MetricFamilyToText(out io.Writer, in *dto.MetricFamily) (int, error) {
 		case dto.MetricType_SUMMARY:
 			if metric.Summary == nil {
 				return written, fmt.Errorf(
-					"expected summary in metric %s", metric,
+					"expected summary in metric %s %s", name, metric,
 				)
 			}
 			for _, q := range metric.Summary.Quantile {
@@ -144,7 +144,7 @@ func MetricFamilyToText(out io.Writer, in *dto.MetricFamily) (int, error) {
 		case dto.MetricType_HISTOGRAM:
 			if metric.Histogram == nil {
 				return written, fmt.Errorf(
-					"expected summary in metric %s", metric,
+					"expected histogram in metric %s %s", name, metric,
 				)
 			}
 			infSeen := false
@@ -191,7 +191,7 @@ func MetricFamilyToText(out io.Writer, in *dto.MetricFamily) (int, error) {
 			)
 		default:
 			return written, fmt.Errorf(
-				"unexpected type in metric %s", metric,
+				"unexpected type in metric %s %s", name, metric,
 			)
 		}
 		written += n

text/parse.go

@@ -83,10 +83,17 @@ type Parser struct {
 // and exactly the same label set), the resulting MetricFamily will contain
 // duplicate Metric proto messages. Similar is true for duplicate label
 // names. Checks for duplicates have to be performed separately, if required.
+// Also note that neither the metrics within each MetricFamily are sorted nor
+// the label pairs within each Metric. Sorting is not required for the most
+// frequent use of this method, which is sample ingestion in the Prometheus
+// server. However, for presentation purposes, you might want to sort the
+// metrics, and in some cases, you must sort the labels, e.g. for consumption by
+// the metric family injection hook of the Prometheus registry.
 //
-// Summaries are a rather special beast. You would probably not use them in the
+// Summaries and histograms are rather special beasts. You would probably not
-// simple text format anyway. This method can deal with summaries if they are
+// use them in the simple text format anyway. This method can deal with
-// presented in exactly the way the text.Create function creates them.
+// summaries and histograms if they are presented in exactly the way the
+// text.Create function creates them.
 //
 // This method must not be called concurrently. If you want to parse different
 // input concurrently, instantiate a separate Parser for each goroutine.