Feature/prometheusv2 #478
Feature/prometheusv2 #478
Conversation
Codecov Report
@@ Coverage Diff @@
## master #478 +/- ##
==========================================
- Coverage 72.31% 62.41% -9.91%
==========================================
Files 132 93 -39
Lines 9703 6667 -3036
==========================================
- Hits 7017 4161 -2856
- Misses 2272 2273 +1
+ Partials 414 233 -181
Continue to review full report at Codecov.
|
|
Could you add a gauge for p99 as well? |
|
@jonathon-L, yea sure I'll add that in. Before I do further work, I'll await to get some feedback. |
|
hey @liclac @robingustafsson, Any thoughts on this and #415? Thanks, |
|
I'm not sure if this is in-scope for this PR, but ultimately exposing these metrics in histogram format would provide the most value. Gauges are fine for graphing an individual test. However, comparing tests over time requires storing the metrics for each request, or at least storing histogram data of each request. |
|
@V3ckt0r Great, thanks for contributing this! Apologies for taking so long before reviewing. I think this PR looks preferable to #415 for the reasons discussed in that PR (avoiding an extra HTTP call to There's some issues we need to solve though in terms of the metrics data. First, the statistics that k6 outputs through the Sink interface are global; The second issue relates to Given this, I think the most appropriate would be, as @jonathon-L said above, to expose |
|
Woops, sorry I must admit I dropped the ball with this @robingustafsson @jonathon-L 😵 I see what you mean about the way the Sink interface handles the percentiles/avg/med etc. I didn't appreciate this before so thought just taking them as is was correct. I agree that getting Prometheus to handle this is better and using Histograms to do this is the way forwards, as @jonathon-L said. Correct me if I'm wrong but I think the second point you raised out about In terms of the use case for this functionality. How do you envisage this working? Are you thinking that the metrics will be tagged with the particular endpoint that is tested. For instance something like |
|
Thanks a lot for this PR(and the one before it ;)). |
|
yea I'll brush off the cobwebs and try pick this back up 😅 |
…et the metrics, instead of make use of the internal api as 415 does.
V3ckt0r
force-pushed
the
feature/prometheusV2
branch
from
40203bf
to
43c6ea4
Compare
|
|
| ) | ||
|
|
||
| var ( | ||
| metrics = make([]v1.Metric, 0) |
There was a problem hiding this comment.
metrics is a global variable (from gochecknoglobals)
| @@ -0,0 +1,229 @@ | |||
| package prometheus | |||
There was a problem hiding this comment.
vendor/github.com/prometheus/client_model/go/metrics.pb.go:96:37: InternalMessageInfo not declared by package proto (from typecheck)
| metrics = make([]v1.Metric, 0) | ||
| ) | ||
|
|
||
| func HandlePrometheusMetrics() http.Handler { |
There was a problem hiding this comment.
exported function HandlePrometheusMetrics should have comment or be unexported (from golint)
| }) | ||
| } | ||
|
|
||
| //prometheus exporter |
There was a problem hiding this comment.
comment on exported type Exporter should be of the form "Exporter ..." (with optional leading article) (from golint)
|
|
||
| "github.com/cloudflare/cfssl/log" | ||
| "github.com/loadimpact/k6/api/common" | ||
| "github.com/loadimpact/k6/api/v1" |
There was a problem hiding this comment.
File is not goimports-ed (from goimports)
|
rebased @mstoykov |
|
Any interest in moving this forward again? |
|
@SuperQ at least for the moment we are not inclined to add more outputs, as #1064 could basically solve all our problems without the need to support and develop all the outputs on our own. This is not definitive "No", but is a "No" for now and probably until we have time to look into #1064 and properly evaluate it. You can at this point use telegraf or some other tool (for example) to translate from some our currently supported outputs (most likely influxdb) to prometheus. |
|
I would suggest moving to Prometheus/OpenMetrics compatible metrics as a base for your instrumentation. This is the direction that InfluxDB is going, as they are also adopting OpenMetrics. When using InfluxDB and Telegraf, it already knows how to ingest Prometheus/OpenMetrics. This means that neither system needs any kind of adapter. |
|
@SuperQ, thanks for mentioning it. There is a separate issue where we can discuss switching to/adding support for OpenMetrics: #858. I admit that I still haven't familiarized myself with the concepts there, but I'll mention it in the telegraf issue (#1064) as something we should evaluate before sinking a lot of time. Still, "OpenMetrics is coming soon" makes me a bit hesitant to bet on it... |
|
While "OpenMetrics" is still technically a draft, it's based on the widely adopted Prometheus format that has been stable since 2014. For the most part, it's 99% the same as the current Prometheus data model, with a few small extensions and some minor cleanup of the specification. |
|
Thanks everybody for working on this, especially @V3ckt0r 🙇 k6 has had support for output extensions for some time now. As of today it also has a template repo for output extensions. This plus all the other extension outputs should be enough for people to extract this code in an extension and use it. Given that I am closing this PR. |
This is an alternative PR to #415 using the K6 engine directly to transpose metrics to Prometheus.