Feature/monitor #1792
Feature/monitor #1792
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #1792 +/- ##
==========================================
+ Coverage 71.43% 71.52% +0.09%
==========================================
Files 178 182 +4
Lines 13777 13891 +114
==========================================
+ Hits 9841 9936 +95
- Misses 3323 3329 +6
- Partials 613 626 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
HI @szkiba, sorry for the slow response and the lint problems :(. We are nearing the v0.30.0 release so we have some stuff we need to finish up, and also catching up after the holidays :).
Let's start with that I am somewhat of the opinion that this should be an output (extension) ... which just has an endpoint (possibly on another port that isn't the REST API). Unfortunately, we currently don't have output extensions, although hopefully work on it will start in the v0.31 cycle, no promises though ;) And I don't know if you can get the Engine from an output, but I am also not certain it is such a good idea to do it that way(see my comment on races).
Your use case is interesting but IMO for this to be in k6 it should be possible to give you all the metrics split by each tag that has been emitted, which is a much bigger change ... including a lot of other things that need to be changed inside of k6 if not for any other reasons but for performance. This is closely related to #1321 and likely fixing that will fix my issue with this PR 🤷
The major and obvious blocker though is that you updated way too many dependencies IMO and as #1758 shows this might not be ... a good idea ;). Can you change the PR to only add the stuff you need instead of updating stuff as sarama, the kafka library we use for another output?
The above doesn't mean that this will get or not get merged based on any of the changes that I have proposed ;). This will need more reviews and figuring out if this change (in any of its forms) will be useful given everything else that is planned in k6, and given the fact that we are very likely (IMO) to change significantly how metrics are handled internally - either making this change one more thing to refactor or something that will be much easier to do.
| case stats.Counter: | ||
| counter := m.Sink.(*stats.CounterSink) | ||
| metrics = append(metrics, newCounter(m.Name+"_count", counter.Value, m.Name+" cumulative value")) | ||
| metrics = append(metrics, newGauge(m.Name+"_rate", counter.Value/(float64(t)/float64(time.Second)), | ||
| m.Name+" value per seconds")) | ||
| case stats.Gauge: | ||
| gauge := m.Sink.(*stats.GaugeSink) | ||
| metrics = append(metrics, newGauge(m.Name+"_value", gauge.Value, m.Name+" latest value")) | ||
| case stats.Trend: | ||
| trend := m.Sink.(*stats.TrendSink) | ||
| trend.Calc() | ||
| metrics = append(metrics, newGauge(m.Name+"_min", trend.Min, m.Name+" minimum value")) | ||
| metrics = append(metrics, newGauge(m.Name+"_max", trend.Max, m.Name+" maximum value")) | ||
| metrics = append(metrics, newGauge(m.Name+"_avg", trend.Avg, m.Name+" average value")) | ||
| metrics = append(metrics, newGauge(m.Name+"_med", trend.Med, m.Name+" median value")) | ||
| metrics = append(metrics, newGauge(m.Name+"_p90", trend.P(0.90), m.Name+" 90 percentile value")) | ||
| metrics = append(metrics, newGauge(m.Name+"_p95", trend.P(0.95), m.Name+" 95 percentile value")) | ||
| case stats.Rate: | ||
| rate := m.Sink.(*stats.RateSink) | ||
| metrics = append(metrics, newGauge(m.Name+"_rate", float64(rate.Trues)/float64(rate.Total), | ||
| m.Name+" percentage of non-zero values")) | ||
| } |
There was a problem hiding this comment.
I think that we should just make everything into a GAUGE ;).
I am not familiar with the prometheus API and conventions, but if this is the way things need to be done ... I don't think it will be all that useful ...
There was a problem hiding this comment.
Probably counter fits better
| for _, m := range engine.Metrics { | ||
| metrics = append(metrics, newMetricFamily(m, t)...) | ||
| } | ||
|
|
||
| data, err := marshallMetricFamily(metrics) |
There was a problem hiding this comment.
This is definetely racy ... there is a reason the Engine has a MetricsLock ;)
You can run go run -race . run script.js and then call the endpoint to get the actual race stacks ... and I would guess there will be others :(.
p.s. the -race flag needs a lot more time to start and it is less performant, but it particularly useful in such situations
There was a problem hiding this comment.
Thank you, I just made it similar to metrics endpoint and didn't spent too much time on it...
Thank you, you review was useful. Probably the k6 extension (the xk6 thing) is a good direction. I didn't realized it, because its a new feature (I read blogs about one years ago). It seem very promising. An I'm happy to see the gRPC support, it is also new for me. Congratulations, k6 become better and better! |
|
Hi @mstoykov , I got a few hours free time to rewrite prometheus http exporter as xk6 extension. I found a cool new feature in k6 release notes: output extension. It is really cool, congrat to k6 team! Using this feature, I reimplemented the exporter as output extension and works fine. https://github.com/szkiba/xk6-prometheus |
|
Prometheus HTTP exporter rewrote as xk6 output extension so this PR is obsolote now. |
Add /v1/monitor endpoint to serve metrics in Prometheus text format.
See #1787