Add Percentile support #86

zhiburt · 2022-10-07T13:58:23Z

Hi there

I doubt I've done the right thing...
But I tried.

close #72

Signed-off-by: Maxim Zhiburt <[email protected]>

zhiburt · 2022-10-07T14:32:56Z

I guess it should consider all scenarious?

kursataktas

Thank you @zhiburt for this work. I've added my comments.

I think we don't need to display percentiles on realtime data.
I guess p99, p95, p90, and p80 would be enough for now.
Let's add a new flag to display/calculate percentiles as optional. Since percentile calculation would be time-consuming.

Example (CLI):
ddosify -t target.com --output-percentile

Example (Config JSON):

{
    "request_count": 10,
    "load_type": "linear",
    "duration": 5,
    "output": "stdout-json",
    "output_percentile": true,
    ...
}

You may need to change the Init() contract of the Report service to pass the new flag to report service implementations.

Example outputs on different scenarios;

1 Step stdout
Multi Step stdout
1 Step stdout-json

{
    "success_perc": 100,
    "fail_perc": 0,
    "success_count": 10,
    "fail_count": 0,
    "avg_duration": 0.225,
    "percentiles": {
          "p99": 0.123,
          "p95": 0.321,
          "p90": 0.33,
          "p80": 0.51
     },
    "steps": {
      "1": {
        "name": "D-1",
        "status_code_dist": {
          "200": 10
        },
        "error_dist": {
          
        },
        "durations": {
          "connection": 0.009,
          "dns": 0.001,
          "request_write": 0,
          "response_read": 0,
          "server_processing": 0.095,
          "tls": 0.01,
          "total": 0.116
        },
        "percentiles": {  // all percentiles are calculated on durations.total value.
          "p99": 0.123,
          "p95": 0.321,
          "p90": 0.33,
          "p80": 0.51
        },
        "success_count": 10,
        "fail_count": 0,
        "success_perc": 100,
        "fail_perc": 0
      }
    }
  }

Multi Step stdout-json

{
  "success_perc": 100,
  "fail_perc": 0,
  "success_count": 10,
  "fail_count": 0,
  "avg_duration": 0.225,
  "total_duration_percentiles": {
        "p99": 0.123,
        "p95": 0.321,
        "p90": 0.33,
        "p80": 0.51
   },
  "steps": {
    "1": {
      "name": "D-1",
      "status_code_dist": {
        "200": 10
      },
      "error_dist": {
        
      },
      "durations": {
        "connection": 0.009,
        "dns": 0.001,
        "request_write": 0,
        "response_read": 0,
        "server_processing": 0.095,
        "tls": 0.01,
        "total": 0.116
      },
      "total_duration_percentiles": {  // all percentiles are calculated on durations.total value.
        "p99": 0.123,
        "p95": 0.321,
        "p90": 0.33,
        "p80": 0.51
      },
      "success_count": 10,
      "fail_count": 0,
      "success_perc": 100,
      "fail_perc": 0
    },
    "2": {
      "name": "D-2",
      "status_code_dist": {
        "200": 10
      },
      "error_dist": {
        
      },
      "durations": {
        "connection": 0.008,
        "dns": 0.001,
        "request_write": 0,
        "response_read": 0,
        "server_processing": 0.09,
        "tls": 0.009,
        "total": 0.109
      },
      "total_duration_percentiles": {  // all percentiles are calculated on durations.total value.
        "p99": 0.123,
        "p95": 0.321,
        "p90": 0.33,
        "p80": 0.51
      },
      "success_count": 10,
      "fail_count": 0,
      "success_perc": 100,
      "fail_perc": 0
    }
  }
}

I wonder what you think about these?

core/report/aggregator.go

…tile

Signed-off-by: Maxim Zhiburt <[email protected]>

zhiburt · 2022-10-18T21:23:36Z

Everything must be addressed.

kursataktas

Thank you for this new design. It looks great 💯

I put my words for the related sections
There are some failed tests, we have to take a look at them
We should add more tests to cover new changes
We may want to update the example_config to show the usage.

kursataktas · 2022-10-19T12:33:59Z

config/json.go

@@ -182,6 +183,7 @@ func (j *JsonReader) CreateHammer() (h types.Hammer, err error) {
 Scenario: s,
 Proxy: p,
 ReportDestination: j.Output,
+ ReportPercentiles: j.OutputPercentile,


Some places use "Percentile" others use "Percentiles". I think we can use "Percentile" everywhere.

Or "Percentiles". Whatever you want, but let's make the naming consistent

kursataktas · 2022-10-19T12:40:01Z

main.go

@@ -74,6 +74,8 @@ var (
 certPath = flag.String("cert_path", "", "A path to a certificate file (usually called 'cert.pem')")
 certKeyPath = flag.String("cert_key_path", "", "A path to a certificate key file (usually called 'key.pem')")

+ outputPercentile = flag.Bool("output-percentile", false, "Report percentile")


"Report percentile" --> "Report percentile(P99, P95, P90, P80)"

kursataktas · 2022-10-19T12:54:06Z

core/report/stdout.go

- red(fmt.Sprintf("%s Failed Run: %-6d %3d%% %5s",
- emoji.CrossMark, s.result.FailedCount, s.result.failedPercentage(), "")),
- blue(fmt.Sprintf("%s Avg. Duration: %.5fs", emoji.Stopwatch, s.result.AvgDuration)))
+ green(fmt.Sprintf("%s Successful Run: %-6d %3d%% %5s", emoji.CheckMark, s.result.SuccessCount, s.result.successPercentage(), "")),


The max. line-length is limited to 120 chars as stated in the linter settings (at .golangci.yml). Let's follow the standards.

kursataktas · 2022-10-21T12:50:46Z

core/report/aggregator.go


+ for _, report := range result.ItemReports {
+ for key, duration := range report.Durations {
+ report.TotalDurations[key] = append(report.TotalDurations[key], duration)


We don't need to store other durations (DNS, TLS, etc.) total duration would be enough ("duration") for now.

kursataktas · 2022-10-21T12:53:23Z

core/report/aggregator.go


+ for _, report := range result.ItemReports {


We don't need this extra loop.

We can add below code snippet at line 57 or anywhere in that else statement

if percentileReportEnabled { item.TotalDurations["duration"] = append(item.TotalDurations["duration"], float32(rr.Duration.Seconds())) }

I think the aggregator should store durations only if the percentage report is enabled.

kursataktas · 2022-10-21T14:19:54Z

core/report/aggregator.go

+
+ n := int(math.Round((float64(p) / 100.0) * float64(len(list))))
+ if n > 0 {
+ // I am not sure about the case where n == 0


No need to cover this unless we need a percentile less than p50, we can remove this check.

kursataktas · 2022-10-21T15:26:16Z

core/report/stdoutJson.go

+
+ if s.reportPercentiles {
+ jsonResult.ItemReports[key].Percentiles = map[string]float32{
+ "p99": item.DurationPercentile(99),


Milliseconds would be enough.

Current stdout-json example:

... "percentiles": { "p80": 0.12210735, --> "0.122" would be enough "p90": 0.14628321, "p95": 0.1905153, "p99": 0.1905153 }, ...

kursataktas · 2022-10-21T15:33:54Z

core/report/stdoutJson.go

+ FailedCount: item.FailedCount,
+ }
+
+ if s.reportPercentiles {


If possible, let's not show the percentiles key if it is not enabled.

Current state:

... "durations": { "connection": 0.005, "dns": 0.009, "request_write": 0, "response_read": 0.382, "server_processing": 0.253, "tls": 0.012, "total": 0.661 }, "percentiles": null, ---> Let's remove this if we can "success_count": 10, "fail_count": 0 }, ...

zhiburt added 2 commits October 7, 2022 16:56

Add Percentile support

e165ab9

Signed-off-by: Maxim Zhiburt <[email protected]>

Merge scenarius when doing p(95)

2cb7be8

Signed-off-by: Maxim Zhiburt <[email protected]>

kursataktas reviewed Oct 17, 2022

View reviewed changes

core/report/aggregator.go Outdated Show resolved Hide resolved

zhiburt added 3 commits October 18, 2022 22:23

Merge branch 'master' of github.com:ddosify/ddosify into patch-percen…

da6968d

…tile

Report percentile only on report stage + add CLI flags

97cbdd4

Signed-off-by: Maxim Zhiburt <[email protected]>

Fix ordering of percentiles in stdout

6a39bcd

Signed-off-by: Maxim Zhiburt <[email protected]>

zhiburt requested a review from kursataktas October 19, 2022 06:30

kursataktas reviewed Oct 21, 2022

View reviewed changes

fatihbaltaci marked this pull request as draft December 6, 2022 16:14

pelohsoe approved these changes Sep 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Percentile support #86

Add Percentile support #86

zhiburt commented Oct 7, 2022

zhiburt commented Oct 7, 2022

kursataktas left a comment •

edited

zhiburt commented Oct 18, 2022 •

edited

kursataktas left a comment

kursataktas Oct 19, 2022

kursataktas Oct 21, 2022

kursataktas Oct 19, 2022

kursataktas Oct 19, 2022

kursataktas Oct 21, 2022

kursataktas Oct 21, 2022

kursataktas Oct 21, 2022 •

edited

kursataktas Oct 21, 2022

kursataktas Oct 21, 2022

kursataktas Oct 21, 2022 •

edited

Add Percentile support #86

Are you sure you want to change the base?

Add Percentile support #86

Conversation

zhiburt commented Oct 7, 2022

zhiburt commented Oct 7, 2022

kursataktas left a comment • edited

Choose a reason for hiding this comment

zhiburt commented Oct 18, 2022 • edited

kursataktas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kursataktas Oct 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kursataktas Oct 21, 2022 • edited

Choose a reason for hiding this comment

kursataktas left a comment •

edited

zhiburt commented Oct 18, 2022 •

edited

kursataktas Oct 21, 2022 •

edited

kursataktas Oct 21, 2022 •

edited