This makes the collisions a bit less likely by XOR'ing descIDs rather
than adding them up for the collectorID.
Signed-off-by: beorn7 <beorn@grafana.com>
Flush is another of the methods that will call WriteHeader if it
hasn't happened yet. Since we want to call observeWriteHeader (if
set), we need to do the WriteHeader call already here, similar to what
we have done in Write and ReadFrom.
This commit also adds comments explaining the above to not tempt
developers to remove the WriteHeader call.
Signed-off-by: beorn7 <beorn@grafana.com>
It is perfectly possible that a normal GC happens just before the
forced one. Thus seeing 2 GCs is fine.
Whenever this test failed, it was because two GCs were seen.
Signed-off-by: beorn7 <bjoern@rabenste.in>
This is really lame as it essentially just uses longer times to
wait. The test is still timing-dependent and thus could still
theoretically fail with unlucky scheduling. However, we are testing
something that _is_ about timing. Turning this all into something not
timing-dependent would be first quite involved and second might defeat
the purpose of testing code that is inherently about timing.
Let's see how this works out in practice.
Signed-off-by: beorn7 <bjoern@rabenste.in>
We stopped advertising binary-wide setting of a label quite a while
ago. This doc comment was missed in the cleanup.
Signed-off-by: beorn7 <bjoern@rabenste.in>
tl;dr: Return previous memstats if reading new ones takes longer than
1s.
See the doc comment of NewGoCollector for details.
Signed-off-by: beorn7 <bjoern@rabenste.in>
The previous `float64(math.MaxUint64 + 1)` is too close to
`float64(math.MaxUint64)` to actually overflow as indended.
The counter code is actually converting forward and backward and
compare the original and twice-converted value. On most platform, this
will create a deviation and thus trigger the expected behavior. By
sheer "luck", one might end up with the same value and thus still use
the uint64 representation. Which is OK within the precision we can
expect. But it breaks the test. With this change, the next
representable floating point value greater than the floating point
value used to represent math.MaxUint64 is used.
Signed-off-by: Bjoern Rabenstein <bjoern@rabenste.in>
This allows us to simplify a bunch of code while still supporting the
last four Go minor versions.
We have also run into minor annoyances a couple of times by now to
keep supporting 1.7 and 1.8.
It's time to pull the plug!
Signed-off-by: Bjoern Rabenstein <bjoern@rabenste.in>
The context package is available since Go 1.7 which is the minimal version
supported by client_golang.
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
reflect.DeepEqual is not suitable for zero occurrences of repeated
proto messages. This changes the comparison to act on the string
representation of proto messages.
Signed-off-by: beorn7 <beorn@soundcloud.com>
So that also users of those can benefit. Obviously, we will end
updating deprecated functions one day (at latest once v0.10 is out).
Signed-off-by: beorn7 <beorn@soundcloud.com>
This is an attempt to expose
https://github.com/istio/istio/issues/8906 . The failure to do so
makes me believe the error is either already fixed in current
client_golang, or something weird I haven't spotted yet is happening
in the istio code.
Signed-off-by: beorn7 <beorn@soundcloud.com>
it appears there is a copy/paste error in the exponential buckets test failure message which is fixed here.
Signed-off-by: David Worth <dworth@strava.com>
This is in line with
https://prometheus.io/docs/instrumenting/writing_clientlibs/#metric-description-and-help
Since the zero value of a string in Go is `""`, we cannot distinguish
between a Help string not set and an empty Help string. Thus, we just
make it formally optional here with an encouragement to set it in the
doc comment.
In v0.10, the Help string will probably become a "normal" argument of
the constructor rather than a field in an Opts struct.
Signed-off-by: beorn7 <beorn@soundcloud.com>
It now uses the new WrapWith function instead of ConstLabels. Describe
is now implemented via DescribeByCollect.
Signed-off-by: beorn7 <beorn@soundcloud.com>
This unifies both constructors in one with an options argument.
The options argument allows to switch on error reporting, as discussed
in #219.
The change of the contructor signature is breaking, but only mildly
so. Plus, the process collector is rarely used explicitly. I used
Sourcegraph to search for public usages, with the following results:
- 2 occurrences of NewProcessCollectorPIDFn, once in @discordianfish's
glimpse, once in @fabxc's etcd_exporter (deprecated anyway). Both
are Prom veterans and will simply do the one line change if needed.
- 8 occurrences of NewProcessCollector, of which 7 are of the form
NewProcessCollector(os.Getpid(), "")
Thus, it's a very easy change, which I even hinted at in the doc
comment.
Signed-off-by: beorn7 <beorn@soundcloud.com>
The only known external usage of it was in prometheus/pushgateway,
where it was removed by
https://github.com/prometheus/pushgateway/pull/200 .
Originally, the expectation was that users would implement the Metric
interface now and then. As we know now, neither it is happening, nor
would it make a lot of sense. (Users implement the Collector interface
instead.) By now, LabelPairSorter is essentially noise in the already
quite cluttered namespace in the prometheus package.
Signed-off-by: beorn7 <beorn@soundcloud.com>
This is for types we don't want to export but which are used in
different packages within client_golang.
Currently, that's only NormalizeMetricFamilies (used in the prometheus
package and in the testutil package). More to be added as needed.
Signed-off-by: beorn7 <beorn@soundcloud.com>
- Expected text format is now read from an io.Reader.
- Metrics are gathered from a Gatherer.
- Added a convenience wrapper to collect from a Collector.
Signed-off-by: beorn7 <beorn@soundcloud.com>
`testutil` is more in line with stdlib naming conventions.
The package should be below `prometheus` as it only provides utils to
test exposition code, not to test HTTP client code.
Signed-off-by: beorn7 <beorn@soundcloud.com>
So far, if a gauge was named `xxx_count`, and a summary or histogram
`xxx`, this would have led to a legal protobuf exposition but would
have created a name collision on `xxx_count` in the text format and
within the Prometheus server.
Signed-off-by: beorn7 <beorn@soundcloud.com>
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 <beorn@soundcloud.com>
While not strictly correct, it can easily happen that proto messages
are created that use nil pointers instead of pointers in empty strings
to denote an empty string.
Signed-off-by: beorn7 <beorn@soundcloud.com>
Fixes:
http.go:118: declaration of "part" shadows declaration at http.go:117
http_test.go:50: declaration of "respBody" shadows declaration at http_test.go:25
promhttp/http.go:305: declaration of "part" shadows declaration at promhttp/http.go:304
Signed-off-by: Karsten Weiss <knweiss@gmail.com>
The test case requires the /proc filesystem. The change prevents this skip
message during "go test -v" on platforms other than Linux:
=== RUN TestProcessCollector
--- SKIP: TestProcessCollector (0.00s)
process_collector_test.go:15: skipping TestProcessCollector, procfs not available: could not read /proc: stat /proc: no such file or directory
Signed-off-by: Karsten Weiss <knweiss@gmail.com>
Fixes:
prometheus/http_test.go:117:5⚠️ should use resp.Body.String() instead of string(resp.Body.Bytes()) (S1030) (megacheck)
prometheus/http_test.go:118:56⚠️ should use resp.Body.String() instead of string(resp.Body.Bytes()) (S1030) (megacheck)
Signed-off-by: Karsten Weiss <knweiss@gmail.com>
Finally, I found an easy solution to provide the "evil"
auto-registration without getting death threats from the wardens of Go
purity. The reasoning can be found in the package's doc comment.
In principle, we needed to iterate through all permutations, mirroring
the same that is happening in the code. For lack of time, I only
picked one of the cases currently buggy.
As said, this really needs code generation, should we ever find
ourselves touching this again.
Previously, the pickDelegator function was not returning a
*hijackerDelegator so the return value did not implement the Hijacker
interface. As a result, code that attempts to hijack the connection
would fail when using a type assertion.
All the other cases returned the hijackerDelegator correctly.
Original discussion see
https://github.com/prometheus/client_golang/pull/362 .
Assuming that the most frequently used method of a `Gauge` is `Set`
and the most frequently used method of a `Conuter` is `Inc`, this
separates the implementation of both metric types. `Inc` and integral
`Add` of a counter is now handled in a separate `uint64`. This would
create a race in `Set`, but luckily, there is no `Set` anymore in a
counter.
All attempts to solve above race (to use the same idea for a `Gauge`)
slow down `Set`, So we just stick with the old implementation
(formerly `value`) for `Gauge`.
The idea behind it is described in detail in
https://github.com/prometheus/client_golang/issues/320 .
This commit also updates the example given in
promhttp/instrument_server_test.go , which nicely illustrates the
benefit of this change.
So far, currying could be emulated by creating different metric vec's
with different values in their ConstLabels. This was quite difficult
to grasp - which is essentially what was done in the example mentioned
above. Now that this use case can be solved without ConstLabels, we
can safely declare ConstLabels as rarely used. (Perhaps we can
deprecate them entirely one day, but I'll take a raincheck on that
when the changes of v0.10 have materialized.) This commit thus also
updates the ConstLabel doc comments in the various Opts. (It contained
fairly outdated stuff anyway.)
The "panic in case of error" code was so far in metricVec. This pulls
it up into the exported types like CounterVec. This is code
replication, but it avoids an explicit type conversion. Mostly,
however, this is preparation to make the wrapped metricVec an
interface (required for curried vec's).
Specifically @beorn7 pointed out that the previous implementation had
some shortcomings around large numbers. I've changed the code to match
the suggestion in review, as well as added a few test cases.
MetricVec is un-experted by now. godoc handles that correctly by
showing the methods of the embedded un-exported metricVec with the
exported type (CounterVec, SummaryVec, ...) that embeds metricVec.
This is in preparation for "curried" metric vecs, as discussed.
And it's a good thing anyway. The exported MetricVec was from a time
when I thought people would define own Metric types and then create
Vecs of it. That has never happened.
As it turned out, it's not that esay to guess "common" combination of
interface upgrades. So I decided to just implement all 32 possible
combination of interface upgrades. (Only 16 with Go 1.7 and earlier.)
Clearly, this calls for code generation. But right now, we still need
to find out what's the best form of the code. For later additions,
implementing code generation might be useful.
Note that newDelegator is called for each HTTP request. Thus, this
commit aims to make the upgrade selection quick. (After the type
checks, it's just directly accessing an element in a slice.)
The example method is assumed to be used as main() function. As a main()
function doesn't have any return values, the example doesn't compile and
is invalid.
This also updates all tests and examples to use explicitly set
objectives.
In v0.10, DefObjectives will be completely removed, and the default
Summary will have no objectives then.
Fixes#118
This finally makes the (presumably) common simple case as simple as it
gets.
The still quite common (but less common) case of using a Gauge is
slightly more verbose now, but not needing to provide a separate
constructor is totally worth it.
Finally, the advanced use case is not really more verbose as in my
original suggestion. However, the logic to decide which Observer to
use is now all in the ObserverFunc handed in at construction
time. This is deliberate and desired. It makes sure the selection
mechanism is all spelled out there. No surprises buried deep in the
function code somewhere.
- Deprecate Untyped for direct instrumentation.
- Add a SetToCurrentTime method to Gauge
Note that adding the SetToCurrentTime method is not really following
Go's principle of lean interfaces. However, the Gauge interface is
already quite fat. (The only methods really required are Set and
Add. Everything else could be expressed in terms of those two.) So we
have already quite a few "convenience" methods traditionally, so I
think we should stay consistent here.
The alternatives would be:
- Not support SetToCurrentTime at all (it's only a SHOULD in the
guidelines).
- A top level function `SetToCurrentTime(Gauge)`.
- Just a helper `CurrentTime()` that returns the curent unix time in
seconds as a float (which is pretty verbose using the standard
library, see code in this commit). This would allow
`myGauge.Set(CurrentTime)`.
Weighing all circumstances, I believe the way in this commit is the
least evil. Issue #223 could be used to rework interfaces more
fundamentally in a breaking change if feasible.
All was a mess, we had duplicates of the REs for label name and metric
names here, and we sometimes used them, sometimes we used those from
common/model.
Now we are consistently using the fast checking functions from common/model.
(Tests for leading colons are included there, see
https://github.com/prometheus/common/pull/66 .)
That's the "soft" part of the deprecation: Everything that has been
marked deprecated in v0.8 or earlier and is straight-forward to
replace by a non-deprecated way, is removed here.
Sadly, this does not include the HTTP part. We first need to provide a
replacement for HTTP instrumentation (as planned for v0.8) to then
remove the deprecated parts in v0.9.
After increasing unit test coverage, it was found that the split
function call nature of metric matching wasn't working well in many
cases. By increasing test coverage, we've ensured that both the fast
path and fallback collision path are working appropriately.
With these changes, there is a further performance hit, but now the
results are ensured to be correct.
Signed-off-by: Stephen J Day <stephen.day@docker.com>
While hash collisions are quite rare, the current state of the client
library carries a risk of merging two separate label values into a
single metric bucket. The effects are near impossible to detect and the
result will be missing or incorrect counters.
This changeset handles hash collisions by falling back to collision
resolution if multiple label values hash to the same value. This works
similar to separate chaining using a slice. Extra storage is minimized
to only the value key slice to that metrics can be differentiated
within a bucket.
In general, the cost of handling collisions is completely minimized
under normal operation. Performance does show slight increases in
certain areas, but these are more likely statistically anomalies. More
importantly, zero allocation behavior for metrics is preserved on the
fast path. Minimal allocations may be made during collision handling but
this has minimal effect.
Benchmark comparisons with and without collision resolution follow.
```
benchmark old ns/op new ns/op delta
BenchmarkCounterWithLabelValues-4 99.0 107 +8.08%
BenchmarkCounterWithLabelValuesConcurrent-4 79.6 91.0 +14.32%
BenchmarkCounterWithMappedLabels-4 518 542 +4.63%
BenchmarkCounterWithPreparedMappedLabels-4 127 137 +7.87%
BenchmarkCounterNoLabels-4 19.5 19.1 -2.05%
BenchmarkGaugeWithLabelValues-4 97.4 110 +12.94%
BenchmarkGaugeNoLabels-4 12.4 10.3 -16.94%
BenchmarkSummaryWithLabelValues-4 1204 915 -24.00%
BenchmarkSummaryNoLabels-4 936 847 -9.51%
BenchmarkHistogramWithLabelValues-4 147 147 +0.00%
BenchmarkHistogramNoLabels-4 50.6 49.3 -2.57%
BenchmarkHistogramObserve1-4 37.9 37.5 -1.06%
BenchmarkHistogramObserve2-4 122 137 +12.30%
BenchmarkHistogramObserve4-4 310 352 +13.55%
BenchmarkHistogramObserve8-4 691 729 +5.50%
BenchmarkHistogramWrite1-4 3374 3097 -8.21%
BenchmarkHistogramWrite2-4 5310 5051 -4.88%
BenchmarkHistogramWrite4-4 12094 10690 -11.61%
BenchmarkHistogramWrite8-4 19416 17755 -8.55%
BenchmarkHandler-4 11934304 13765894 +15.35%
BenchmarkSummaryObserve1-4 1119 1105 -1.25%
BenchmarkSummaryObserve2-4 3679 3430 -6.77%
BenchmarkSummaryObserve4-4 10678 7982 -25.25%
BenchmarkSummaryObserve8-4 22974 16689 -27.36%
BenchmarkSummaryWrite1-4 25962 14680 -43.46%
BenchmarkSummaryWrite2-4 38019 35073 -7.75%
BenchmarkSummaryWrite4-4 78027 56816 -27.18%
BenchmarkSummaryWrite8-4 117220 132248 +12.82%
BenchmarkMetricVecWithLabelValuesBasic-4 138 133 -3.62%
BenchmarkMetricVecWithLabelValues2Keys10ValueCardinality-4 150 144 -4.00%
BenchmarkMetricVecWithLabelValues4Keys10ValueCardinality-4 263 256 -2.66%
BenchmarkMetricVecWithLabelValues2Keys100ValueCardinality-4 145 155 +6.90%
BenchmarkMetricVecWithLabelValues10Keys100ValueCardinality-4 606 634 +4.62%
BenchmarkMetricVecWithLabelValues10Keys1000ValueCardinality-4 746 641 -14.08%
benchmark old allocs new allocs delta
BenchmarkCounterWithLabelValues-4 0 0 +0.00%
BenchmarkCounterWithLabelValuesConcurrent-4 0 0 +0.00%
BenchmarkCounterWithMappedLabels-4 2 2 +0.00%
BenchmarkCounterWithPreparedMappedLabels-4 0 0 +0.00%
BenchmarkCounterNoLabels-4 0 0 +0.00%
BenchmarkGaugeWithLabelValues-4 0 0 +0.00%
BenchmarkGaugeNoLabels-4 0 0 +0.00%
BenchmarkSummaryWithLabelValues-4 0 0 +0.00%
BenchmarkSummaryNoLabels-4 0 0 +0.00%
BenchmarkHistogramWithLabelValues-4 0 0 +0.00%
BenchmarkHistogramNoLabels-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValuesBasic-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues2Keys10ValueCardinality-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues4Keys10ValueCardinality-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues2Keys100ValueCardinality-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues10Keys100ValueCardinality-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues10Keys1000ValueCardinality-4 0 0 +0.00%
benchmark old bytes new bytes delta
BenchmarkCounterWithLabelValues-4 0 0 +0.00%
BenchmarkCounterWithLabelValuesConcurrent-4 0 0 +0.00%
BenchmarkCounterWithMappedLabels-4 336 336 +0.00%
BenchmarkCounterWithPreparedMappedLabels-4 0 0 +0.00%
BenchmarkCounterNoLabels-4 0 0 +0.00%
BenchmarkGaugeWithLabelValues-4 0 0 +0.00%
BenchmarkGaugeNoLabels-4 0 0 +0.00%
BenchmarkSummaryWithLabelValues-4 0 0 +0.00%
BenchmarkSummaryNoLabels-4 0 0 +0.00%
BenchmarkHistogramWithLabelValues-4 0 0 +0.00%
BenchmarkHistogramNoLabels-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValuesBasic-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues2Keys10ValueCardinality-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues4Keys10ValueCardinality-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues2Keys100ValueCardinality-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues10Keys100ValueCardinality-4 0 0 +0.00%
BenchmarkMetricVecWithLabelValues10Keys1000ValueCardinality-4 0 0 +0.00%
```
Signed-off-by: Stephen J Day <stephen.day@docker.com>
This allows to finally get rid of the infamous injection hook in the
interface. The old SetMetricFamilyInjectionHook still exist as a
deprecated function but is now implemented with the new plumbing under
the hood.
Now that we have multiple Gatherer implementation, I renamed
push.Registry to push.FromGatherer.
This commit also improves the consistency checks, which happened as a
byproduct of the refactoring to allow checking in both the "merge
gatherer" Gatherers as well as in the normal Registry.
To keep backwards compatibility while not creating circular import
chains, some code had to be duplicated. But all functions using it
have been declared deprecated hereby.
The new ways of instrumenting handlers will all go into the new
package, and ultimately, the prometheus package itself will be
completely igorant of HTTP.
- Moved the Deliverer parameter to the end of the list to mirror
Collectors in push.Collectors.
- Improved doc comment and added an example for push.Registry.
Registry is now a struct, which implements two interfaces, Registrerer
and Deliverer. The latter is particularly important as it is now the
argument type for pushes and HTTP handler construction (i.e. it is
easy to implement a custom Deliverer for testing or other
purposes). The Registerer interface is not used as a parameter type
but can (and should) be used by users of custom registries so that
they can easily do things like mocking it out for testing purposes.
With the broken up interfaces, adding MustRegister to the interface is
not such a big deal anymore (interface is still small). And since
setting the injection hook is such a rare thing to happen, it is
acceptable to not have it in any of the interfaces.
The renaming from `Collect` to `Deliver` was done to avoid confusion
with Collectors. (The registry _collects_ from the Collectors, and
then _delivers_ to the exposition mechanism.)
General context and approch
===========================
This is the first part of the long awaited wider refurbishment of
`client_golang/prometheus/...`. After a lot of struggling, I decided
to not go for one breaking big-bang, but cut things into smaller steps
after all, mostly to keep the changes manageable and easy to
review. I'm aiming for having the invasive breaking changes
concentrated in as few steps as possible (ideally one). Some steps
will not be breaking at all, but typically there will be breaking
changes that only affect quite special cases so that 95+% of users
will not be affected. This first step is an example for that, see
details below.
What's happening in this commit?
================================
This step is about finally creating an exported registry
interface. This could not be done by simply export the existing
internal implementation because the interface would be _way_ too
fat. This commit introduces a qutie lean `Registry` interface
(compared to the previous interval implementation). The functions that
act on the default registry are retained (with very few exceptions) so
that most use cases won't see a change. However, several of those are
deprecated now to clean up the namespace in the future.
The default registry is kept in the public variable
`DefaultRegistry`. This follows the example of the http package in the
standard library (cf. `http.DefaultServeMux`, `http.DefaultClient`)
with the same implications. (This pattern is somewhat disputed within
the Go community but I chose to go with the devil you know instead of
creating something more complex or even disallowing any changes to the
default registry. The current approach gives everybody the freedom to
not touch DefaultRegistry or to do everything with a custom registry
to play save.)
Another important part in making the registry lean is the extraction
of the HTTP exposition, which also allows for customization of the
HTTP exposition. Note that the separation of metric collection and
exposition has the side effect that managing the MetricFamily and
Metric protobuf objects in a free-list or pool isn't really feasible
anymore. By now (with better GC in more recent Go versions), the
returns were anyway dimisishing. To be effective at all, scrapes had
to happen more often than GC cycles, and even then most elements of
the protobufs (everything excetp the MetricFamily and Metric structs
themselves) would still cause allocation churn. In a future breaking
change, the signature of the Write method in the Metric interface will
be adjusted accordingly. In this commit, avoiding breakage is more
important.
The following issues are fixed by this commit (some solved "on the
fly" now that I was touching the code anyway and it would have been
stupid to port the bugs):
https://github.com/prometheus/client_golang/issues/46https://github.com/prometheus/client_golang/issues/100https://github.com/prometheus/client_golang/issues/170https://github.com/prometheus/client_golang/issues/205
Documentation including examples have been amended as required.
What future changes does this commit enable?
============================================
The following items are not yet implemented, but this commit opens the
possibility of implementing these independently.
- The separation of the HTTP exposition allows the implementation of
other exposition methods based on the Registry interface, as known
from other Prometheus client libraries, e.g. sending the metrics to
Graphite.
Cf. https://github.com/prometheus/client_golang/issues/197
- The public `Registry` interface allows the implementation of
convenience tools for testing metrics collection. Those tools can
inspect the collected MetricFamily protobufs and compare them to
expectation. Also, tests can use their own testing instance of a
registry.
Cf. https://github.com/prometheus/client_golang/issues/58
Notable non-goals of this commit
================================
Non-goals that will be tackled later
------------------------------------
The following two issues are quite closely connected to the changes in
this commit but the line has been drawn deliberately to address them
in later steps of the refurbishment:
- `InstrumentHandler` has many known problems. The plan is to create a
saner way to conveniently intrument HTTP handlers and remove the old
`InstrumentHandler` altogether. To keep breakage low for now, even
the default handler to expose metrics is still using the old
`InstrumentHandler`. This leads to weird naming inconsistencies but
I have deemed it better to not break the world right now but do it
in the change that provides better ways of instrumenting HTTP
handlers.
Cf. https://github.com/prometheus/client_golang/issues/200
- There is work underway to make the whole handling of metric
descriptors (`Desc`) more intuitive and transparent for the user
(including an ability for less strict checking,
cf. https://github.com/prometheus/client_golang/issues/47). That's
quite invasive from the perspective of the internal code, namely the
registry. I deliberately kept those changes out of this commit.
- While this commit adds new external dependency, the effort to vendor
anything within the library that is not visible in any exported
types will have to be done later.
Non-goals that _might_ be tackled later
---------------------------------------
There is a strong and understandable urge to divide the `prometheus`
package into a number of sub-packages (like `registry`, `collectors`,
`http`, `metrics`, …). However, to not run into a multitude of
circular import chains, this would need to break every single existing
usage of the library. (As just one example, if the ubiquitious
`prometheus.MustRegister` (with more than 2,000 uses on GitHub alone)
is kept in the `prometheus` package, but the other registry concerns
go into a new `registry` package, then the `prometheus` package would
import the `registry` package (to call the actual register method),
while at the same time the `registry` package needs to import the
`prometheus` package to access `Collector`, `Metric`, `Desc` and
more. If we moved `MustRegister` into the `registry` package,
thousands of code lines would have to be fixed (which would be easy if
the world was a mono repo, but it is not). If we moved everything else
the proposed registry package needs into packages of their own, we
would break thousands of other code lines.)
The main problem is really the top-level functions like
`MustRegister`, `Handler`, …, which effectively pull everything into
one package. Those functions are however very convenient for the easy
and very frequent use-cases.
This problem has to be revisited later.
For now, I'm trying to keep the amount of exported names in the
package as low as possible (e.g. I unexported expvarCollector in this
commit because the NewExpvarCollector constructor is enough to export,
and it is now consistent with other collectors, like the goCollector).
Non-goals that won't be tackled anytime soon
--------------------------------------------
Something that I have played with a lot is "streaming collection",
i.e. allow an implementation of the `Registry` interface that collects
metrics incrementally and serves them while doing so. As it has turned
out, this has many many issues and makes the `Registry` interface very
clunky. Eventually, I made the call that it is unlikely we will really
implement streaming collection; and making the interface more clunky
for something that might not even happen is really a big no-no. Note
that the `Registry` interface only creates the in-memory
representation of the metric family protobufs in one go. The
serializaton onto the wire can still be handled in a streaming fashion
(which hasn't been done so far, without causing any trouble, but might
be done in the future without breaking any interfaces).
What are the breaking changes?
==============================
- Signatures of functions pushing to Pushgateway have changed to allow
arbitrary grouping (which was planned for a long time anyway, and
now that I had to work on the Push code anyway for the registry
refurbishment, I finally did it,
cf. https://github.com/prometheus/client_golang/issues/100).
With the gained insight that pushing to the default registry is almost
never the right thing, and now that we are breaking the Push call
anyway, all the Push functions were moved to their own package,
which cleans up the namespace and is more idiomatic (pushing
Collectors is now literally done by `push.Collectors(...)`).
- The registry is doing more consistency checks by default now. Past
creators of inconsistent metrics could have masked the problem by
not setting `EnableCollectChecks`. Those inconsistencies will now be
detected. (But note that a "best effort" metrics collection is now
possible with `HandlerOpts.ErrorHandling = ContinueOnError`.)
- `EnableCollectChecks` is gone. The registry is now performing some
of those checks anyway (see previous item), and a registry with all
of those checks can now be created with `NewPedanticRegistry` (only
used for testing).
- `PanicOnCollectError` is gone. This behavior can now be configured
when creating a custom HTTP handler.
This test fails when max_fds is a large value; say 4.5e+06, for example. Change it to match virtual_memory_bytes, which also has to handle large values.
This circumvents the following problem:
• The prometheus client library appends “/metrics/…” to the pushURL.
• When pushURL ends in a trailing slash, the URL becomes e.g.
http://pushgateway.example.com:9091//metrics/…
• The pushgateway will reply with an HTTP 307 status code
(temporary redirect).
• While Go’s net/http client follows redirects by default, it will only
follow HTTP 302 (Found) and HTTP 303 (See Other) redirects for PUT and
POST requests, which the prometheus client library uses.
Hence, when calling e.g.:
prometheus.Push("foo", "bar", "http://pushgateway.example.com:9091/")
…your metrics would not actually get pushed successfully, but rather
you’d see the error message:
2015/11/26 10:59:49 main.go:209: unexpected status code 307 while pushing to http://push...
beorn7