These tests are always timing out in our Jenkins CI environment. We have moved our timeout to 2 minutes. I have added a check for the short flag so other can skip these tests as we are until this can be identified.
We are running into a timeout with TestHistogramConcurrency on our Jenkins box. I noticed in the stack trace for the timeout this block.
goroutine 2348 [chan send]:
github.comcast.com/ventris/kober/vnd/github.com/prometheus/client_golang/prometheus.(*goCollector).Collect(0xc20801e8e0, 0xc20800a7e0)
/var/lib/jenkins/jobs/Kober/workspace/src/github.comcast.com/ventris/kober/vnd/github.com/prometheus/client_golang/prometheus/go_collector.go:49 +0x6dd
github.comcast.com/ventris/kober/vnd/github.com/prometheus/client_golang/prometheus.func·028()
/var/lib/jenkins/jobs/Kober/workspace/src/github.comcast.com/ventris/kober/vnd/github.com/prometheus/client_golang/prometheus/go_collector_test.go:27 +0x11a
created by github.comcast.com/ventris/kober/vnd/github.com/prometheus/client_golang/prometheus.TestGoCollector
/var/lib/jenkins/jobs/Kober/workspace/src/github.comcast.com/ventris/kober/vnd/github.com/prometheus/client_golang/prometheus/go_collector_test.go:28 +0x35e
This suggested that even though the TestGoCollector test was finished, a goroutine was still hanging around. I traced it back to the call to c.Collect always sending twice of the provided channel. This change receives that second value and allows the goroutine to finish with the test.
Still can't figure out why TestHistogramConcurrency is timing out after 2 minutes :(
- 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.)
InstrumentHandler provides proxy object over given http.ResponseWriter
- responseWriterDelegator that only implements the bare minimum
required of an http.ResponseWriter and doesn’t implement any interface
upgrades (http.Flusher, http.Hijacker, etc.). This commit fixes it by
providing fancyResponseWriterDelegator with all the fancy bells and
whistles if standard library http.ResponseWriter is detected.
Heavily inspired by Goji's middleware.
https://avtok.com/2014/11/05/interface-upgrades.html#the-proxy-problem
The cgo dependency in the "procfs" package is being removed in:
https://github.com/prometheus/procfs/pull/4
So now it should be fine to always import the "procfs" package and have
it determine dynamically at runtime whether the proc filesystem is
actually there.
This fixes a problem for users which were vendoring client_golang on
MacOS X, but building the result on Linux (the procfs package was
missing in the vendored packages).
If a metric family returned by the injection hook already exists (with
the same name), then its metrics are simply merged into that metric
family. With enabled collect-time checks, even uniqueness is checked,
but in general, things stay the same that the caller is responsible to
ensure metric consistency.
This fixes https://github.com/prometheus/pushgateway/issues/27 .
The new vendoring was produced by running:
godep save -r ./examples/... ./prometheus/... ./text/... ./model/... ./extraction/...
Two things to note:
- "extraction/processor0_0_{1,2}_test.go" imported a package from
"github.com/prometheus/prometheus", all for just one tiny testing
function. To not have to deal with a circular vendoring dependency, I
simply replaced the usage of the function by some in-line logic.
- godep grouped the rewritten imports slightly differently for some
reason, but at least the standard library imports are still in a
separate section. Not sure if it's worth manually keeping our old
import grouping scheme or if we should simply use that godep-generated
one.
As it is expected that the process collector can fail under certain
conditions (proc information for a process only readable by root or
other user for example) and as there is currently no option to configure
the error behavior of the client, this change reverts the error
reporting introduced in 159e96f. This effectively means that errors are
simply ignored and there won't be any samples for the process_* metrics
in case of an error.
Once a user can control how to behave in case of errors returned by
collectors, this change should probably be reverted.
The procfs package has a cgo dependency (necessary to calculate system
times). As procfs is not available under windows, darwin and supposely
all newer BSD systems, this change remove the procfs dependency on these
systems.
If a given pidFn for a process collector can't determine the pid, this
had to be signaled with an invalid pid so far (pid <= 0). In order to
make the error interface clearer for users, this change introduces an
explicit error parameter.
Adjust the API and usage accordingly.
Make tests stricter.
Since the merging is still faulty, test are broken now.
The next commit will fix it by avoiding merging.
Summaries as implemented cannot be aggregated in a meaningful
way. Partitoning them by status code and method only made sense if we
were interested in the individual latency and size of e.g. GET request
that result in status 503. In general, that's not the case. Most of
the time, the user will be interested in the latency and size of _all_
HTTP requests.
(With future changes to client_golang, we will consider making the
HTTP instrumentation configurable, e.g. to handle the case where the
user is only interested in the latency of successful requests.)
Both are interface changes I want to get in before public
announcement. They only break rare usage cases, and are always easy to
fix, but still we want to avoid breaking changes after a wider
announcement of the project.
The change of Register() simply removes the return of the Collector,
which nobody was using in practice. It was just bloating the call
syntax. Note that this is different from RegisterOrGet(), which is
used at various occasions where you want to register something that
might or might not be registered already, but if it is, you want the
previously registered Collector back (because that's the relevant
one).
WRT error reporting: I first tried the obvious way of letting the
Collector methods Describe() and Collect() return error. However, I
had to conclude that that bloated _many_ calls and their handling in
very obnoxious ways. On the other hand, the case where you actually
want to report errors during registration or collection is very
rare. Hence, this approach has the wrong trade-off. The approach taken
here might at first appear clunky but is in practice quite handy,
mostly because there is almost no change for the "normal" case of "no
special error handling", but also because it plays well with the way
descriptors and metrics are handled (via channels).
Explaining the approach in more detail:
- During registration / describe: Error handling was actually already
in place (for invalid descriptors, which carry an error anyway). I
only added a convenience function to create an invalid descriptor
with a given error on purpose.
- Metrics are now treated in a similar way. The Write method returns
an error now (the only change in interface). An "invalid metric" is
provided that can be sent via the channel to signal that that metric
could not be collected. It alse transports an error.
NON-GOALS OF THIS COMMIT:
This is NOT yet the major improvement of the whole registry part,
where we want a public Registry interface and plenty of modular
configurations (for error handling, various auto-metrics, http
instrumentation, testing, ...). However, we can do that whole thing
without breaking existing interfaces. For now (which is a significant
issue) any error during collection will either cause a 500 HTTP
response or a panic (depending on registry config). Later, we
definitely want to have a possibility to skip (and only report
somehow) non-collectible metrics instead of aborting the whole scrape.
This change adds two new collectors to the prometheus package which
export metrics about a given or the current process.
* ProcessCollector exports metrics about cpu time, vss, rss, fd usage as
well as the start time of a given process.
* GoCollector exports currently only the number of active goroutines.
computeApproximateRequestSize is run in a goroutine, but the
handlerFunc that runs in parallel may modify the URL, which is also
needed by computeApproximateRequestSize. So get the URL length
beforehand.
Change-Id: Idb84735845afe7be4ef79b3d642d5764f6d26a7c
Also, remove quotes from the Content-type header. It's not illegal to
have quotes there, but they are not needed, and at other places, we
are not using them. So fewer characters and more consistency.
Change-Id: If7a78bde85154163e4426daec493d973213e83e9
Since we prepare the whole content in a buf before sending, we can as
well set the Content-Length explicitly.
Change-Id: Ifd91764c90af53be49f93f0b33032138130b6f96
This is actually the intended behavior, and (as a nice side effect)
makes things cheaper to calculate.
Also, introduce a separator character to avoid hash collisions
(like label values {"ab","c"} vs {"a", "bc"}).
Apply the same principles to signature.go.
Change-Id: I607db544f278ed89684fe5fa11abdbc3e03d3061
Also, fix seconds to microseconds fot the http instrumentation to
match the metric name.
Fix Desc.String().
Simplify http error display.
Change-Id: Ib7397f4eac1eeed92b291e1c9cc88c080aee99ca
This rewrite had may backs and forths. In my git repository, it
consists of 35 commits which I cannot group or merge into reasonable
review buckets. Gerrit breaks fundamental git semantics, so I have to
squash the 35 commits into one for the review.
I'll push this not with refs/for/master, but with refs/for/next so
that we can transition after submission in a controlled fashion.
For the review, I recommend to start with looking at godoc and in
particular the many examples. After that, continue with a line-by-line
detailed review. (The big picture is hopefully as expected after
wrapping up the discussion earlier.)
Change-Id: Ib38cc46493a5139ca29d84020650929d94cac850
Most important here is the simple & flat text format, but while I'm on
it, I have also added the text representations for protobufs (which is
purely meant for debugging purposes). I hope my basic idea about
handling those various protocols (and the text package) becomes
clearer now.
Change-Id: I7299853eadc82a426101e907f2b3d4e37f9e4c71
The LabelsToSignature function is now used outside of the prometheus
package, too. Leaving it in the prometheuos package is misleading
design and will lead to circulat import chains soon.
Change-Id: If1ca442d4023b33b138cf79fee68e82ff2a355be
- Full support for UNTYPED type.
- Receptive support for timestamp_ms (i.e. the processor can process
it, but the client library cannot yet create it - which is kind of
intended as timestamps are meant for other things like federation,
which will need separate support anyway).
Change-Id: I5913164a80089943d49ad58bf86e465a843ab82b
The idea here is to always go via the protobufs if dealing with the
text format. That won't always be the most efficient way, but it
avoids the multiplicity of conversion routines required for direct
conversion (e.g. text format -> internal representation in the
Prometheus server). The loss of efficiency is acceptable because the
text format should not be used in high performance (high throughput,
low latency) situations anyway.
In that way, the text format stays perfectly isolated from other parts
of the code. To receive text format, just plug the conversion in
before the code path that normally reads protobufs. Correspondingly,
for sending text format, simply replace the WriteDelimited call by a
text.Create call.
Nevertheless, the conversion code itself is optimized for efficiency
and minimized memory churn (which was one of the reason for handcoding
the parser and not using a lexer/parser code generation tool).
Change-Id: Iee45ffe8aa421a844225d13a1f859becd8a3b066
These are all simple changes we should have caught a long time ago:
1. The hashing mechanism for fingerprint label sets should have not
allocated new objects for the actual hashing---at least not
egregiously. This simplifies the hash writing by just byte-
dumping the string stream into the hasher.
2. The hashing mechanism within the scope of a metric does not care
about the value of the label keys themselves but only of the label
values. The keys can be dropped from the calculation.
3. The locking mechanism for the metrics should not block on hash
computation but rather solely on the actual mutation or critical
section reads.
4. For scalar metrics (i.e., ones with niladic label signatures), we
should rely on a preallocated map versus requesting a new one
ad hoc.
This is tested with Go 1.1, so the results may yield other values
for us elsewhere:
BEFORE
BenchmarkLabelValuesToSignatureScalar 500000000 3.97 ns/op 0 B/op 0 allocs/op
BenchmarkLabelValuesToSignatureSingle 5000000 714 ns/op 74 B/op 4 allocs/op
BenchmarkLabelValuesToSignatureDouble 1000000 1153 ns/op 107 B/op 5 allocs/op
BenchmarkLabelValuesToSignatureTriple 1000000 1588 ns/op 138 B/op 6 allocs/op
BenchmarkLabelToSignatureScalar 500000000 3.91 ns/op 0 B/op 0 allocs/op
BenchmarkLabelToSignatureSingle 2000000 874 ns/op 92 B/op 5 allocs/op
BenchmarkLabelToSignatureDouble 1000000 1528 ns/op 139 B/op 7 allocs/op
BenchmarkLabelToSignatureTriple 1000000 2172 ns/op 186 B/op 9 allocs/op
AFTER
BenchmarkLabelValuesToSignatureScalar 500000000 4.36 ns/op 0 B/op 0 allocs/op
BenchmarkLabelValuesToSignatureSingle 5000000 378 ns/op 89 B/op 4 allocs/op
BenchmarkLabelValuesToSignatureDouble 5000000 574 ns/op 142 B/op 5 allocs/op
BenchmarkLabelValuesToSignatureTriple 5000000 758 ns/op 186 B/op 6 allocs/op
BenchmarkLabelToSignatureScalar 500000000 4.06 ns/op 0 B/op 0 allocs/op
BenchmarkLabelToSignatureSingle 5000000 472 ns/op 106 B/op 5 allocs/op
BenchmarkLabelToSignatureDouble 2000000 746 ns/op 174 B/op 7 allocs/op
BenchmarkLabelToSignatureTriple 1000000 1061 ns/op 235 B/op 9 allocs/op
In effect, a single metric mutation operation's lookup overhead will
move from Before::iBenchmarkLabelToSignature to
After::BenchmarkLabelValuesToSignature. This MINIMALLY reduces
1/2 the overhead. I would be hesitant in reading the memory
allocation statistics, for this was run with the GC still on and
thusly inaccurate per Go benchmarking documentation.
Before::BenchmarkLabelValuesToSignature never existed, so it is not
of any intrinsic value in itself. That said, the cases that still
rely on LabelToSignature experience consistently a 1/2 drop in time.
Change-Id: Ifc9e69f718af65a59f5be8117473518233258159
Kevin Pike