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

Add Percentile support #86

Closed
wants to merge 5 commits into from
Closed

Conversation

zhiburt
Copy link
Contributor

@zhiburt zhiburt commented Oct 7, 2022

Hi there

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

image

close #72

Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
Signed-off-by: Maxim Zhiburt <zhiburt@gmail.com>
@zhiburt
Copy link
Contributor Author

zhiburt commented Oct 7, 2022

I guess it should consider all scenarious?

image

Copy link
Member

@kursataktas kursataktas left a comment

Choose a reason for hiding this comment

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

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

  1. I think we don't need to display percentiles on realtime data.
  2. I guess p99, p95, p90, and p80 would be enough for now.
  3. 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
    image

  • Multi Step stdout
    image

  • 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 Outdated Show resolved Hide resolved
@zhiburt
Copy link
Contributor Author

zhiburt commented Oct 18, 2022

Everything must be addressed.

image

Copy link
Member

@kursataktas kursataktas left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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

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(), "")),
Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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


for _, report := range result.ItemReports {
Copy link
Member

Choose a reason for hiding this comment

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

We don't need this extra loop.

Copy link
Member

@kursataktas kursataktas Oct 21, 2022

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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
      },
...

FailedCount: item.FailedCount,
}

if s.reportPercentiles {
Copy link
Member

@kursataktas kursataktas Oct 21, 2022

Choose a reason for hiding this comment

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

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
    },
...

@fatihbaltaci fatihbaltaci marked this pull request as draft December 6, 2022 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: quantile/percentile based statistics
4 participants