percentiles chart #66

brenol · 2020-10-12T19:15:54Z

This PR adds percentiles chart support, which can be enabled with Ctrl-(P) ercentiles.

You can change to latencies chart again Latencies chart with Ctrl-(L) atencies.

As I'm not sure we're "hiding" this functionality under a shortcut, I did not update the README yet.

Added a utility function to create charts with legends. (preparing for bytes-in/bytes-out or any other metric)
Added a utility type so we can easily create charts with legends (chartLegend)

Although it's working correctly, I few like there are a few improvements that can be made (specially in gui/gui.go).

This PR also fixes a few issues I noticed while using ali:

Percentiles (text) where not being updated correctly. This was happening because vegeta.Metrics only set P50/90... when (*vegeta.Metrics).Close() is called. I modified newMetrics to calculate quantiles as newMetrics is called.
Also fixed the handling of chartCh and metrics.
The way it was, it could not calculate floating point values (as when appending it calculated value/time.Millisecond. As time.Millisecond is an int64, it means everything was being rounded to either 0s or 1.0s), not allowing us to calculate 500ms as 0.5s.

It also allows texts to be customized (with cellColors - this is necessary because of Legends, which we want to have the same color as its own Series).

This is how it looks:

I feel like adding values to charts is way too manual, at the moment. Perhaps adding a type that holds a function which extracts metrics (from vegeta.Metrics) to a lineChart is a good idea?

sync

…s called

codecov-io · 2020-10-12T19:16:51Z

Codecov Report

Merging #66 into master will decrease coverage by 2.12%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   77.87%   75.75%   -2.11%     
==========================================
  Files           8        8              
  Lines         393      536     +143     
==========================================
+ Hits          306      406     +100     
- Misses         68       99      +31     
- Partials       19       31      +12

Impacted Files	Coverage Δ
gui/keybinds.go	`51.62% <64.71%> (+11.62%)`	⬆️
gui/widgets.go	`60.98% <67.65%> (-1.09%)`	⬇️
attacker/attacker.go	`71.02% <70.59%> (+1.79%)`	⬆️
gui/gui.go	`72.64% <84.79%> (-0.28%)`	⬇️
attacker/metrics.go	`100.00% <100.00%> (ø)`
gui/drawer.go	`93.82% <100.00%> (+1.82%)`	⬆️
main.go	`75.20% <0.00%> (-5.70%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86bc7fb...cc7c887. Read the comment docs.

nakabonne · 2020-10-12T23:24:13Z

@brenol Hey, I'm very grateful for your support. I can’t afford to check it, so I'll get back to you later :-)

gui/gui.go

nakabonne · 2020-10-17T01:42:27Z

This PR adds percentiles chart support, which can be enabled with Ctrl-(P) ercentiles.
You can change to latencies chart again Latencies chart with Ctrl-(L) atencies.

Actually, I'd like to make it capable of switching to the next chart by pressing l, and press h to go back to the previous chart (vim style).
For instance, whenever you press l, it will switch to:

Latency
Percentile
Bytes (to be supported)
Latency

Whenever you press h:

Latency
Bytes
Percentile

Percentiles (text) where not being updated correctly. This was happening because vegeta.Metrics only set P50/90... when (*vegeta.Metrics).Close() is called. I modified newMetrics to calculate quantiles as newMetrics is called.

The way it was, it could not calculate floating point values (as when appending it calculated value/time.Millisecond. As time.Millisecond is an int64, it means everything was being rounded to either 0s or 1.0s), not allowing us to calculate 500ms as 0.5s.

Nice catch! Thanks for all of them!

I feel like adding values to charts is way too manual, at the moment. Perhaps adding a type that holds a function which extracts metrics (from vegeta.Metrics) to a lineChart is a good idea?

I'm planning to address this complexity when we have more chart types.

brenol · 2020-10-17T01:58:13Z

This PR adds percentiles chart support, which can be enabled with Ctrl-(P) ercentiles.
You can change to latencies chart again Latencies chart with Ctrl-(L) atencies.

Actually, I'd like to make it capable of switching to the next chart by pressing l, and press h to go back to the previous chart (vim style).
For instance, whenever you press l, it will switch to:

Latency

Percentile

Bytes (to be supported)

Latency

Whenever you press h:

Latency

Bytes

Percentile

Percentiles (text) where not being updated correctly. This was happening because vegeta.Metrics only set P50/90... when (*vegeta.Metrics).Close() is called. I modified newMetrics to calculate quantiles as newMetrics is called.
The way it was, it could not calculate floating point values (as when appending it calculated value/time.Millisecond. As time.Millisecond is an int64, it means everything was being rounded to either 0s or 1.0s), not allowing us to calculate 500ms as 0.5s.

Nice catch! Thanks for all of them!

I feel like adding values to charts is way too manual, at the moment. Perhaps adding a type that holds a function which extracts metrics (from vegeta.Metrics) to a lineChart is a good idea?

I'm planning to address this complexity when we have more chart types.

I like the idea (L/H). I didn't think H/L would work but makes complete sense. Just made a little change (to see if it would work with L/P):

var (
	L = keyboard.Key('L')
	H = keyboard.Key('H')
	l = keyboard.Key('l')
	h = keyboard.Key('h')
)

And it works! (Didn't notice I'd be able to do that, lol)

I'll change to make it work as vim 👍 :)

…H & h)

brenol · 2020-10-17T21:58:26Z

Hey @nakabonne,

Made it work as vim. :)

nakabonne · 2020-10-18T00:21:22Z

attacker/metrics.go

+			P50:   m.Latencies.Quantile(0.50),
+			P90:   m.Latencies.Quantile(0.90),
+			P95:   m.Latencies.Quantile(0.95),
+			P99:   m.Latencies.Quantile(0.99),


I'm running into an issue about data races when running it at a high rate. There seems to be a data race between calculating the quartiles for the chart and calculating them for metrics reporting.

It's preferable to re-use a single quantile, but if it gets too complicated, you may want to revert back to it for now.

Suggested change

P50: m.Latencies.Quantile(0.50),

P90: m.Latencies.Quantile(0.90),

P95: m.Latencies.Quantile(0.95),

P99: m.Latencies.Quantile(0.99),

P50: m.Latencies.P50,

P90: m.Latencies.P90,

P95: m.Latencies.P95,

P99: m.Latencies.P99,

I'm wondering if it's a little too heavy to use the sync.Mutex every time, but I haven't been able to confirm it yet. I'll check it in a few hours.

Interesting data race. I'll have a look at it.

From what I gathered, commenting out sendMetrics actually fix that data race but that is not good. I think a mutex, in the case for performance will actually be OK, but it'll be all around the code:

newMetrics

adding metrics/calculating quantiles

redrawChart, as it accesses Latencies to write metrics

Also, I just hit another data race, which happens with commented sendMetrics + starting a new attack:

WARNING: DATA RACE Read at 0x00c000190718 by goroutine 20: github.com/nakabonne/ali/gui.attack() /home/brenol/goworkspace/src/github.com/brenol/ali/gui/keybinds.go:50 +0x5b github.com/nakabonne/ali/gui.keybinds.func3() /home/brenol/goworkspace/src/github.com/brenol/ali/gui/keybinds.go:40 +0x1d7 github.com/mum4k/termdash.(*termdash).subscribers.func4() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/termdash.go:236 +0x7b github.com/mum4k/termdash/private/event.(*subscriber).callback() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/private/event/event.go:95 +0x5a github.com/mum4k/termdash/private/event.(*subscriber).run() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/private/event/event.go:110 +0xd2 Previous write at 0x00c000190718 by goroutine 52: github.com/nakabonne/ali/gui.(*drawer).redrawChart() /home/brenol/goworkspace/src/github.com/brenol/ali/gui/drawer.go:86 +0x13e8

OK, changed to a Mutex and fixed both data races. (the one you found and the one I found after)

I did not see a performance hit, but I did not use a lot of workers though.

edit

I tried first with a RWMutex but it did not work. Not sure why though.

nakabonne · 2020-10-18T03:54:38Z

gui/drawer.go

 	chartCh   chan *attacker.Result
 	gaugeCh   chan bool
 	metricsCh chan *attacker.Metrics

 	// aims to avoid to perform multiple `redrawChart`.
-	chartDrawing bool
+	chartDrawing int32


nice catch! can you use go.uber.org/atomic.Bool instead of sync/atomic?

https://godoc.org/go.uber.org/atomic#Bool

sure :) I thought of using it but as I wasn't sure if you'd be ok with one more dependency I just left it at stdlib. I'll update shortly

Thanks for your concern :)

Done. Using Uber's atomic.

Also, when running with -race and doing a Ctrl-C, after a attack I believe there is a data race in termdash:

WARNING: DATA RACE Write at 0x000000c5e448 by main goroutine: github.com/nsf/termbox-go.Close() /home/brenol/goworkspace/pkg/mod/github.com/nsf/termbox-go@v0.0.0-20200204031403-4d2b513ad8be/api.go:149 +0x5e8 github.com/mum4k/termdash/terminal/termbox.(*Terminal).Close() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/terminal/termbox/termbox.go:163 +0x55 github.com/nakabonne/ali/gui.Run() /home/brenol/goworkspace/src/github.com/brenol/ali/gui/gui.go:34 +0x230 main.(*cli).run() /home/brenol/goworkspace/src/github.com/brenol/ali/main.go:117 +0x26c main.main() /home/brenol/goworkspace/src/github.com/brenol/ali/main.go:60 +0x104 Previous read at 0x000000c5e448 by goroutine 18: github.com/nsf/termbox-go.flush() /home/brenol/goworkspace/pkg/mod/github.com/nsf/termbox-go@v0.0.0-20200204031403-4d2b513ad8be/termbox.go:242 +0x3e github.com/nsf/termbox-go.Flush() /home/brenol/goworkspace/pkg/mod/github.com/nsf/termbox-go@v0.0.0-20200204031403-4d2b513ad8be/api.go:211 +0x784 github.com/mum4k/termdash/terminal/termbox.(*Terminal).Flush() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/terminal/termbox/termbox.go:113 +0x2f github.com/mum4k/termdash.(*termdash).redraw() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/termdash.go:278 +0x1bd github.com/mum4k/termdash.(*termdash).evRedraw() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/termdash.go:294 +0xa6 github.com/mum4k/termdash.(*termdash).subscribers.func3() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/termdash.go:230 +0x44 github.com/mum4k/termdash/private/event.(*subscriber).callback() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/private/event/event.go:95 +0x5a github.com/mum4k/termdash/private/event.(*subscriber).run() /home/brenol/goworkspace/pkg/mod/github.com/mum4k/termdash@v0.12.2/private/event/event.go:110 +0xd2

Can't do much about that one though, I'll report it to termdash or see if its fixed in a newer version...

Thank you for letting me know. Yup, I could repro and this is obviously a different problem. Let's address it as another issue 👍

Open an issue about it: #69

nakabonne

LTGM 👍 I (and ali as well) greatly appreciate your cooperation!

brenol added 14 commits October 12, 2020 13:10

add percentiles chart

f8a7526

Merge pull request #1 from nakabonne/master

5a13182

sync

add percentiles chart

127297e

add tests

35358a2

use pointer directly

f6c3132

no need to use newMetrics anymore; just calculate after metrics.Add i…

cf3f3a6

…s called

a few renames so its more concise with the rest of the code

58fc946

fix test values

4ba3412

formatting

245c090

handle floating point milliseconds (such as 0.5, 1.5, etc)

71969b4

fix percentiles tests & finish

ca7e827

remove (now) unnecessary cast to float64

9a4c3d9

fix typo & rephrase

15054ed

rename p99/p95/p90 etc so its clear that we are talking about legends

f03a0b2

nakabonne requested changes Oct 17, 2020

View reviewed changes

gui/gui.go Show resolved Hide resolved

add missing error handling

2d5bc7a

nakabonne mentioned this pull request Oct 17, 2020

Plot more metrics in real-time #2

Closed

5 tasks

add the possibility to move forward (with L & l) and backwards (with …

e7c22da

…H & h)

nakabonne reviewed Oct 18, 2020

View reviewed changes

fix found data races

55a02ea

nakabonne reviewed Oct 18, 2020

View reviewed changes

use ubers atomic pkg instead

cc7c887

nakabonne approved these changes Oct 18, 2020

View reviewed changes

nakabonne merged commit 60eba11 into nakabonne:master Oct 18, 2020

brenol deleted the percentiles-chart branch October 18, 2020 04:54

nakabonne mentioned this pull request Oct 18, 2020

Data race in termdash #69

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

percentiles chart #66

percentiles chart #66

brenol commented Oct 12, 2020 •

edited

codecov-io commented Oct 12, 2020 •

edited

nakabonne commented Oct 12, 2020

nakabonne commented Oct 17, 2020

brenol commented Oct 17, 2020

brenol commented Oct 17, 2020

nakabonne Oct 18, 2020 •

edited

nakabonne Oct 18, 2020

brenol Oct 18, 2020

brenol Oct 18, 2020 •

edited

nakabonne Oct 18, 2020

brenol Oct 18, 2020

nakabonne Oct 18, 2020

brenol Oct 18, 2020 •

edited

nakabonne Oct 18, 2020

nakabonne Oct 18, 2020

nakabonne left a comment

percentiles chart #66

percentiles chart #66

Conversation

brenol commented Oct 12, 2020 • edited

codecov-io commented Oct 12, 2020 • edited

Codecov Report

nakabonne commented Oct 12, 2020

nakabonne commented Oct 17, 2020

brenol commented Oct 17, 2020

brenol commented Oct 17, 2020

nakabonne Oct 18, 2020 • edited

Choose a reason for hiding this comment

nakabonne Oct 18, 2020

Choose a reason for hiding this comment

brenol Oct 18, 2020

Choose a reason for hiding this comment

brenol Oct 18, 2020 • edited

Choose a reason for hiding this comment

nakabonne Oct 18, 2020

Choose a reason for hiding this comment

brenol Oct 18, 2020

Choose a reason for hiding this comment

nakabonne Oct 18, 2020

Choose a reason for hiding this comment

brenol Oct 18, 2020 • edited

Choose a reason for hiding this comment

nakabonne Oct 18, 2020

Choose a reason for hiding this comment

nakabonne Oct 18, 2020

Choose a reason for hiding this comment

nakabonne left a comment

Choose a reason for hiding this comment

brenol commented Oct 12, 2020 •

edited

codecov-io commented Oct 12, 2020 •

edited

nakabonne Oct 18, 2020 •

edited

brenol Oct 18, 2020 •

edited

brenol Oct 18, 2020 •

edited