From 59f2c7d8b06707f8cf14fe3b09c4dc61295d09fa Mon Sep 17 00:00:00 2001 From: beorn7 Date: Mon, 8 Jun 2015 22:12:07 +0200 Subject: [PATCH] Fix a number of minor things. - Clarify documentation about sorting requirements. - Add missing histogram support in consistency check. - Add label sorting to consistency check. - Improve error messages when reporting a metric. (Previously, the metric name was not printed.) --- model/metric.go | 2 +- prometheus/registry.go | 40 +++++++++++++++++++++++++------------ prometheus/registry_test.go | 20 +++++++++---------- text/create.go | 12 +++++------ text/parse.go | 13 +++++++++--- 5 files changed, 54 insertions(+), 33 deletions(-) diff --git a/model/metric.go b/model/metric.go index 0870f23..864c012 100644 --- a/model/metric.go +++ b/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) diff --git a/prometheus/registry.go b/prometheus/registry.go index 2515311..3223193 100644 --- a/prometheus/registry.go +++ b/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", - dtoMetric, metricFamily.Type, + "collected metric %s %s is not a %s", + 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", - dtoMetric, + "collected metric %s %s was collected before with the same name and label values", + 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", - dtoMetric, metricFamily.GetName(), desc.fqName, + "collected metric %s %s has name %q but should have %q", + 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", - dtoMetric, metricFamily.GetHelp(), desc.help, + "collected metric %s %s has help %q but should have %q", + 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", - dtoMetric, desc, + "labels in collected metric %s %s are inconsistent with descriptor %s", + 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", - dtoMetric, desc, + "labels in collected metric %s %s are inconsistent with descriptor %s", + 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 diff --git a/prometheus/registry_test.go b/prometheus/registry_test.go index bbf11f6..f30c90c 100644 --- a/prometheus/registry_test.go +++ b/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: counter: > + externalMetricFamilyAsProtoCompactText := []byte(`name:"externalname" help:"externaldocstring" type:COUNTER metric: label: counter: > `) expectedMetricFamily := &dto.MetricFamily{ diff --git a/text/create.go b/text/create.go index 4430459..57def32 100644 --- a/text/create.go +++ b/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 diff --git a/text/parse.go b/text/parse.go index e317d68..2f337a8 100644 --- a/text/parse.go +++ b/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 -// simple text format anyway. This method can deal with summaries if they are -// presented in exactly the way the text.Create function creates them. +// Summaries and histograms are rather special beasts. You would probably not +// use them in the simple text format anyway. This method can deal with +// 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.