From c06fb788be8a05442219295095ee0e51523802f0 Mon Sep 17 00:00:00 2001 From: beorn7 Date: Wed, 6 Jun 2018 19:30:53 +0200 Subject: [PATCH] Relax consistency checks during gathering Also, clarify in the doc comment. Previously, the assumption was that inconsistent label dimensions are violating the exposition format spec. However, especially with the knowledge that OpenMetrics will explicitly allow inconsistent label dimensions in expositions, we should allow it in client_golang, too. Note that registration with proper Descs provided will still check for consistont label dimensions. However, you can "cheat" with custom Collectors as you can collect metrics that don't follew the provided Desc (this will be made more explicit and less cheaty once #47 is fixed). You can also create expositions with inconsistent label dimensions by merging Gatherers with the Gatherers slice type. (The latter is used in the Pushgateway.) Effectively, normal direct instrumentation will always have consistent label dimensions in this way, but you can cover special use cases with custom collectors or merging of different Gatherers. Signed-off-by: beorn7 --- prometheus/examples_test.go | 7 ++--- prometheus/registry.go | 61 +++++++++++++++---------------------- 2 files changed, 27 insertions(+), 41 deletions(-) diff --git a/prometheus/examples_test.go b/prometheus/examples_test.go index 45f6065..9808d5c 100644 --- a/prometheus/examples_test.go +++ b/prometheus/examples_test.go @@ -712,7 +712,7 @@ humidity_percent{location="inside"} 33.2 # HELP temperature_kelvin Temperature in Kelvin. # Duplicate metric: temperature_kelvin{location="outside"} 265.3 - # Wrong labels: + # Missing location label (note that this is undesirable but valid): temperature_kelvin 4.5 ` @@ -740,15 +740,14 @@ temperature_kelvin 4.5 // temperature_kelvin{location="outside"} 273.14 // temperature_kelvin{location="somewhere else"} 4.5 // ---------- - // 2 error(s) occurred: - // * collected metric temperature_kelvin label: gauge: was collected before with the same name and label values - // * collected metric temperature_kelvin gauge: has label dimensions inconsistent with previously collected metrics in the same metric family + // collected metric temperature_kelvin label: gauge: was collected before with the same name and label values // # HELP humidity_percent Humidity in %. // # TYPE humidity_percent gauge // humidity_percent{location="inside"} 33.2 // humidity_percent{location="outside"} 45.4 // # HELP temperature_kelvin Temperature in Kelvin. // # TYPE temperature_kelvin gauge + // temperature_kelvin 4.5 // temperature_kelvin{location="inside"} 298.44 // temperature_kelvin{location="outside"} 273.14 } diff --git a/prometheus/registry.go b/prometheus/registry.go index 94b7b4b..9306cbb 100644 --- a/prometheus/registry.go +++ b/prometheus/registry.go @@ -126,15 +126,21 @@ type Registerer interface { type Gatherer interface { // Gather calls the Collect method of the registered Collectors and then // gathers the collected metrics into a lexicographically sorted slice - // of MetricFamily protobufs. Even if an error occurs, Gather attempts - // to gather as many metrics as possible. Hence, if a non-nil error is - // returned, the returned MetricFamily slice could be nil (in case of a - // fatal error that prevented any meaningful metric collection) or - // contain a number of MetricFamily protobufs, some of which might be - // incomplete, and some might be missing altogether. The returned error - // (which might be a MultiError) explains the details. In scenarios - // where complete collection is critical, the returned MetricFamily - // protobufs should be disregarded if the returned error is non-nil. + // of uniquely named MetricFamily protobufs. Gather ensures that the + // returned slice is valid and self-consistent so that it can be used + // for valid exposition. As an exception to the strict consistency + // requirements described for metric.Desc, Gather will tolerate + // different sets of label names for metrics of the same metric family. + // + // Even if an error occurs, Gather attempts to gather as many metrics as + // possible. Hence, if a non-nil error is returned, the returned + // MetricFamily slice could be nil (in case of a fatal error that + // prevented any meaningful metric collection) or contain a number of + // MetricFamily protobufs, some of which might be incomplete, and some + // might be missing altogether. The returned error (which might be a + // MultiError) explains the details. In scenarios where complete + // collection is critical, the returned MetricFamily protobufs should be + // disregarded if the returned error is non-nil. Gather() ([]*dto.MetricFamily, error) } @@ -370,7 +376,6 @@ func (r *Registry) Gather() ([]*dto.MetricFamily, error) { var ( metricChan = make(chan Metric, capMetricChan) metricHashes = map[uint64]struct{}{} - dimHashes = map[string]uint64{} wg sync.WaitGroup errs MultiError // The collected errors to return in the end. registeredDescIDs map[uint64]struct{} // Only used for pedantic checks @@ -433,7 +438,7 @@ collectLoop: } errs.Append(processMetric( metric, metricFamiliesByName, - metricHashes, dimHashes, + metricHashes, registeredDescIDs, )) default: @@ -445,7 +450,7 @@ collectLoop: for metric := range metricChan { errs.Append(processMetric( metric, metricFamiliesByName, - metricHashes, dimHashes, + metricHashes, registeredDescIDs, )) } @@ -465,7 +470,6 @@ func processMetric( metric Metric, metricFamiliesByName map[string]*dto.MetricFamily, metricHashes map[uint64]struct{}, - dimHashes map[string]uint64, registeredDescIDs map[uint64]struct{}, ) error { desc := metric.Desc() @@ -542,7 +546,7 @@ func processMetric( } metricFamiliesByName[desc.fqName] = metricFamily } - if err := checkMetricConsistency(metricFamily, dtoMetric, metricHashes, dimHashes); err != nil { + if err := checkMetricConsistency(metricFamily, dtoMetric, metricHashes); err != nil { return err } if registeredDescIDs != nil { @@ -584,7 +588,6 @@ func (gs Gatherers) Gather() ([]*dto.MetricFamily, error) { var ( metricFamiliesByName = map[string]*dto.MetricFamily{} metricHashes = map[uint64]struct{}{} - dimHashes = map[string]uint64{} errs MultiError // The collected errors to return in the end. ) @@ -624,7 +627,7 @@ func (gs Gatherers) Gather() ([]*dto.MetricFamily, error) { metricFamiliesByName[mf.GetName()] = existingMF } for _, m := range mf.Metric { - if err := checkMetricConsistency(existingMF, m, metricHashes, dimHashes); err != nil { + if err := checkMetricConsistency(existingMF, m, metricHashes); err != nil { errs = append(errs, err) continue } @@ -701,18 +704,13 @@ func normalizeMetricFamilies(metricFamiliesByName map[string]*dto.MetricFamily) } // checkMetricConsistency checks if the provided Metric is consistent with the -// provided MetricFamily. It also hashed the Metric labels and the MetricFamily +// provided MetricFamily. It also hashes the Metric labels and the MetricFamily // name. If the resulting hash is already in the provided metricHashes, an error -// is returned. If not, it is added to metricHashes. The provided dimHashes maps -// MetricFamily names to their dimHash (hashed sorted label names). If dimHashes -// doesn't yet contain a hash for the provided MetricFamily, it is -// added. Otherwise, an error is returned if the existing dimHashes in not equal -// the calculated dimHash. +// is returned. If not, it is added to metricHashes. func checkMetricConsistency( metricFamily *dto.MetricFamily, dtoMetric *dto.Metric, metricHashes map[uint64]struct{}, - dimHashes map[string]uint64, ) error { // Type consistency with metric family. if metricFamily.GetType() == dto.MetricType_GAUGE && dtoMetric.Gauge == nil || @@ -732,19 +730,18 @@ func checkMetricConsistency( } } - // Is the metric unique (i.e. no other metric with the same name and the same label values)? + // Is the metric unique (i.e. no other metric with the same name and the same labels)? h := hashNew() h = hashAdd(h, metricFamily.GetName()) h = hashAddByte(h, separatorByte) - dh := hashNew() // Make sure label pairs are sorted. We depend on it for the consistency // check. sort.Sort(LabelPairSorter(dtoMetric.Label)) for _, lp := range dtoMetric.Label { + h = hashAdd(h, lp.GetName()) + h = hashAddByte(h, separatorByte) h = hashAdd(h, lp.GetValue()) h = hashAddByte(h, separatorByte) - dh = hashAdd(dh, lp.GetName()) - dh = hashAddByte(dh, separatorByte) } if _, exists := metricHashes[h]; exists { return fmt.Errorf( @@ -752,16 +749,6 @@ func checkMetricConsistency( metricFamily.GetName(), dtoMetric, ) } - if dimHash, ok := dimHashes[metricFamily.GetName()]; ok { - if dimHash != dh { - return fmt.Errorf( - "collected metric %s %s has label dimensions inconsistent with previously collected metrics in the same metric family", - metricFamily.GetName(), dtoMetric, - ) - } - } else { - dimHashes[metricFamily.GetName()] = dh - } metricHashes[h] = struct{}{} return nil }