Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

percentiles chart #66

Merged
merged 18 commits into from
Oct 18, 2020
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion attacker/attacker.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ type Options struct {
// Result contains the results of a single HTTP request.
type Result struct {
Latency time.Duration

P50 time.Duration
P90 time.Duration
P95 time.Duration
P99 time.Duration

// Indicates if the last result in the entire attack.
End bool
}
Expand Down Expand Up @@ -112,8 +118,14 @@ func Attack(ctx context.Context, target string, resCh chan *Result, metricsCh ch
opts.Attacker.Stop()
return
default:
resCh <- &Result{Latency: res.Latency}
metrics.Add(res)
resCh <- &Result{
Latency: res.Latency,
P50: metrics.Latencies.Quantile(0.50),
P90: metrics.Latencies.Quantile(0.90),
P95: metrics.Latencies.Quantile(0.95),
P99: metrics.Latencies.Quantile(0.99),
}
}
}
metrics.Close()
Expand Down
8 changes: 4 additions & 4 deletions attacker/metrics.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@ func newMetrics(m *vegeta.Metrics) *Metrics {
Latencies: LatencyMetrics{
Total: m.Latencies.Total,
Mean: m.Latencies.Mean,
P50: m.Latencies.P50,
P90: m.Latencies.P90,
P95: m.Latencies.P95,
P99: m.Latencies.P99,
P50: m.Latencies.Quantile(0.50),
P90: m.Latencies.Quantile(0.90),
P95: m.Latencies.Quantile(0.95),
P99: m.Latencies.Quantile(0.99),
Comment on lines +79 to +82
Copy link
Owner

@nakabonne nakabonne Oct 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
screenshot-1

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,

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@brenol brenol Oct 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Max: m.Latencies.Max,
Min: m.Latencies.Min,
},
Expand Down
34 changes: 33 additions & 1 deletion gui/drawer.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

type drawer struct {
widgets *widgets
gridOpts *gridOpts
chartCh chan *attacker.Result
gaugeCh chan bool
metricsCh chan *attacker.Metrics
Expand All @@ -27,6 +28,16 @@ type drawer struct {
// TODO: In the future, multiple charts including bytes-in/out etc will be re-drawn.
func (d *drawer) redrawChart(ctx context.Context, maxSize int) {
values := make([]float64, 0, maxSize)

valuesP50 := make([]float64, 0, maxSize)
valuesP90 := make([]float64, 0, maxSize)
valuesP95 := make([]float64, 0, maxSize)
valuesP99 := make([]float64, 0, maxSize)

appendValue := func(to []float64, val time.Duration) []float64 {
return append(to, float64(val)/float64(time.Millisecond))
}

d.chartDrawing = true
L:
for {
Expand All @@ -42,13 +53,34 @@ L:
break L
}
d.gaugeCh <- false
values = append(values, float64(res.Latency/time.Millisecond))

values = appendValue(values, res.Latency)
d.widgets.latencyChart.Series("latency", values,
linechart.SeriesCellOpts(cell.FgColor(cell.ColorNumber(87))),
linechart.SeriesXLabels(map[int]string{
0: "req",
}),
)

valuesP50 = appendValue(valuesP50, res.P50)
d.widgets.percentilesChart.Series("p50", valuesP50,
linechart.SeriesCellOpts(d.widgets.p50Legend.cellOpts...),
)

valuesP90 = appendValue(valuesP90, res.P90)
d.widgets.percentilesChart.Series("p90", valuesP90,
linechart.SeriesCellOpts(d.widgets.p90Legend.cellOpts...),
)

valuesP95 = appendValue(valuesP95, res.P95)
d.widgets.percentilesChart.Series("p95", valuesP95,
linechart.SeriesCellOpts(d.widgets.p95Legend.cellOpts...),
)

valuesP99 = appendValue(valuesP99, res.P99)
d.widgets.percentilesChart.Series("p99", valuesP99,
linechart.SeriesCellOpts(d.widgets.p99Legend.cellOpts...),
)
}
}
d.chartDrawing = false
Expand Down
45 changes: 41 additions & 4 deletions gui/drawer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,54 @@ func TestRedrawChart(t *testing.T) {
defer cancel()

tests := []struct {
name string
results []*attacker.Result
latencyChart LineChart
name string
results []*attacker.Result
latencyChart LineChart
percentilesChart LineChart
}{
{
name: "one result received",
results: []*attacker.Result{
{
Latency: 1000000,
P50: 500000,
P90: 900000,
P95: 950000,
P99: 990000,
},
},
latencyChart: func() LineChart {
l := NewMockLineChart(ctrl)
l.EXPECT().Series("latency", []float64{1.0}, gomock.Any())
return l
}(),
percentilesChart: func() LineChart {
l := NewMockLineChart(ctrl)
gomock.InOrder(
l.EXPECT().Series("p50", []float64{0.5}, gomock.Any()),
l.EXPECT().Series("p90", []float64{0.9}, gomock.Any()),
l.EXPECT().Series("p95", []float64{0.95}, gomock.Any()),
l.EXPECT().Series("p99", []float64{0.99}, gomock.Any()),
)
return l
}(),
},
{
name: "two results received",
results: []*attacker.Result{
{
Latency: 1000000,
P50: 500000,
P90: 900000,
P95: 950000,
P99: 990000,
},
{
Latency: 2000000,
P50: 1000000,
P90: 1800000,
P95: 1900000,
P99: 1980000,
},
},
latencyChart: func() LineChart {
Expand All @@ -51,13 +74,27 @@ func TestRedrawChart(t *testing.T) {
l.EXPECT().Series("latency", []float64{1.0, 2.0}, gomock.Any())
return l
}(),
percentilesChart: func() LineChart {
l := NewMockLineChart(ctrl)

gomock.InOrder(l.EXPECT().Series("p50", []float64{0.5}, gomock.Any()),
l.EXPECT().Series("p90", []float64{0.9}, gomock.Any()),
l.EXPECT().Series("p95", []float64{0.95}, gomock.Any()),
l.EXPECT().Series("p99", []float64{0.99}, gomock.Any()),
l.EXPECT().Series("p50", []float64{0.5, 1.0}, gomock.Any()),
l.EXPECT().Series("p90", []float64{0.9, 1.8}, gomock.Any()),
l.EXPECT().Series("p95", []float64{0.95, 1.9}, gomock.Any()),
l.EXPECT().Series("p99", []float64{0.99, 1.98}, gomock.Any()),
)
return l
}(),
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
d := &drawer{
widgets: &widgets{latencyChart: tt.latencyChart},
widgets: &widgets{latencyChart: tt.latencyChart, percentilesChart: tt.percentilesChart},
chartCh: make(chan *attacker.Result),
gaugeCh: make(chan bool, 100),
}
Expand Down
73 changes: 68 additions & 5 deletions gui/gui.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const (
// How often termdash redraws the screen.
redrawInterval = 250 * time.Millisecond
rootID = "root"
chartID = "chart"
)

type runner func(ctx context.Context, t terminalapi.Terminal, c *container.Container, opts ...termdash.Option) error
Expand Down Expand Up @@ -49,25 +50,66 @@ func run(t *termbox.Terminal, r runner, targetURL string, opts *attacker.Options
if err != nil {
return fmt.Errorf("failed to build grid layout: %w", err)
}
if err := c.Update(rootID, gridOpts...); err != nil {
if err := c.Update(rootID, gridOpts.base...); err != nil {
return fmt.Errorf("failed to update container: %w", err)
}

d := &drawer{
widgets: w,
gridOpts: gridOpts,
chartCh: make(chan *attacker.Result),
gaugeCh: make(chan bool),
metricsCh: make(chan *attacker.Metrics),
}
go d.redrawMetrics(ctx)

k := keybinds(ctx, cancel, d, targetURL, *opts)
k := keybinds(ctx, cancel, c, d, targetURL, *opts)

return r(ctx, t, c, termdash.KeyboardSubscriber(k), termdash.RedrawInterval(redrawInterval))
}

func gridLayout(w *widgets) ([]container.Option, error) {
raw1 := grid.RowHeightPerc(70, grid.Widget(w.latencyChart, container.Border(linestyle.Light), container.BorderTitle("Latency (ms)")))
// newChartWithLegends creates a chart with legends at the bottom.
// TODO: use it for more charts than percentiles. Any chart that has multiple series would be able to use this func.
func newChartWithLegends(lineChart LineChart, opts []container.Option, texts ...Text) ([]container.Option, error) {
textsInColumns := func() []grid.Element {
els := make([]grid.Element, 0, len(texts))
for _, text := range texts {
els = append(els, grid.ColWidthPerc(3, grid.Widget(text)))
}
return els
}

lopts := lineChart.Options()
el := grid.RowHeightPercWithOpts(70,
opts,
grid.RowHeightPerc(97, grid.ColWidthPerc(99, grid.Widget(lineChart))),
grid.RowHeightPercWithOpts(3,
[]container.Option{container.MarginLeftPercent(lopts.MinimumSize.X)},
textsInColumns()...,
),
)

g := grid.New()
g.Add(el)
return g.Build()
}

// gridOpts holds all options in our grid.
// It basically holds the container options (column, width, padding, etc) of our widgets.
type gridOpts struct {
// base options
base []container.Option

// so we can replace containers
latency []container.Option
percentiles []container.Option
}

func gridLayout(w *widgets) (*gridOpts, error) {
raw1 := grid.RowHeightPercWithOpts(70,
[]container.Option{container.ID(chartID)},
grid.Widget(w.latencyChart, container.Border(linestyle.Light), container.BorderTitle("Latency (ms)")),
)
raw2 := grid.RowHeightPerc(25,
grid.ColWidthPerc(20, grid.Widget(w.paramsText, container.Border(linestyle.Light), container.BorderTitle("Parameters"))),
grid.ColWidthPerc(20, grid.Widget(w.latenciesText, container.Border(linestyle.Light), container.BorderTitle("Latencies"))),
Expand All @@ -90,5 +132,26 @@ func gridLayout(w *widgets) ([]container.Option, error) {
raw3,
)

return builder.Build()
baseOpts, err := builder.Build()
if err != nil {
return nil, err
}
latencyBuilder := grid.New()
latencyBuilder.Add(raw1)
latencyOpts, err := latencyBuilder.Build()
if err != nil {
return nil, err
}

percentilesOpts, err := newChartWithLegends(w.percentilesChart, []container.Option{
brenol marked this conversation as resolved.
Show resolved Hide resolved
container.Border(linestyle.Light),
container.ID(chartID),
container.BorderTitle("Percentiles (ms)"),
}, w.p99Legend.text, w.p90Legend.text, w.p95Legend.text, w.p50Legend.text)

return &gridOpts{
latency: latencyOpts,
percentiles: percentilesOpts,
base: baseOpts,
}, nil
}
7 changes: 6 additions & 1 deletion gui/keybinds.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,24 @@ import (
"context"
"time"

"github.com/mum4k/termdash/container"
"github.com/mum4k/termdash/keyboard"
"github.com/mum4k/termdash/terminal/terminalapi"

"github.com/nakabonne/ali/attacker"
)

func keybinds(ctx context.Context, cancel context.CancelFunc, dr *drawer, targetURL string, opts attacker.Options) func(*terminalapi.Keyboard) {
func keybinds(ctx context.Context, cancel context.CancelFunc, c *container.Container, dr *drawer, targetURL string, opts attacker.Options) func(*terminalapi.Keyboard) {
return func(k *terminalapi.Keyboard) {
switch k.Key {
case keyboard.KeyCtrlC: // Quit
cancel()
case keyboard.KeyEnter: // Attack
attack(ctx, dr, targetURL, opts)
case keyboard.KeyCtrlP: // percentiles chart
c.Update(chartID, dr.gridOpts.percentiles...)
case keyboard.KeyCtrlL: // latency chart
c.Update(chartID, dr.gridOpts.latency...)
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion gui/keybinds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func TestKeybinds(t *testing.T) {
}
}
}(ctx)
f := keybinds(ctx, cancel, nil, "", attacker.Options{})
f := keybinds(ctx, cancel, nil, nil, "", attacker.Options{})
f(&terminalapi.Keyboard{Key: tt.key})
// If ctx wasn't expired, goleak will find it.
})
Expand Down
Loading