#164 Fortio remote test URL shouldn't require the percentiles list #179

olimpias · 2018-03-05T20:04:57Z

HistogramData will be experted with empty percentile array to avoid null in json. Also test is added for this.

ldemailly · 2018-03-05T20:11:58Z

sorry the idea of #164 isn't not to deal with nil; that part works fine; the idea is that the UI should have the same default as fortio_main

olimpias · 2018-03-05T20:16:42Z

Ah okay.

ldemailly · 2018-03-05T20:18:38Z

the test for success is:

fortio server &

then hit

http://localhost:8080/fortio/?labels=Fortio&url=http%3A%2F%2Flocalhost%3A8080%2Fecho&t=3s&qps=1000&save=on&r=0.0001&load=Start

it should do "50, 75, 90, 99, 99.9" percentiles; it doesn't now

olimpias · 2018-03-05T20:37:43Z

I have tested to see the case, but it works. You can see the result json below. Percentiles are seen on DurationHistogram data.

{
  "Labels": "Fortio",
  "StartTime": "2018-03-05T23:33:42.873072921+03:00",
  "RequestedQPS": "1000",
  "RequestedDuration": "3s",
  "ActualQPS": 571.8950653941345,
  "ActualDuration": 3012790465,
  "NumThreads": 8,
  "Version": "0.7.3-pre",
  "DurationHistogram": {
    "Count": 1723,
    "Min": 0.001372129,
    "Max": 0.056114808,
    "Sum": 22.966513984000002,
    "Avg": 0.013329375498549043,
    "StdDev": 0.006322253301454129,
    "Data": [
      {
        "Start": 0.001372129,
        "End": 0.0014,
        "Percent": 0.05803830528148578,
        "Count": 1
      },
      {
        "Start": 0.0016,
        "End": 0.0018000000000000002,
        "Percent": 0.2321532211259431,
        "Count": 3
      },
      {
        "Start": 0.002,
        "End": 0.0025,
        "Percent": 0.522344747533372,
        "Count": 5
      },
      {
        "Start": 0.0025,
        "End": 0.003,
        "Percent": 0.6964596633778294,
        "Count": 3
      },
      {
        "Start": 0.003,
        "End": 0.0035,
        "Percent": 0.7544979686593152,
        "Count": 1
      },
      {
        "Start": 0.0035,
        "End": 0.004,
        "Percent": 0.8705745792222868,
        "Count": 2
      },
      {
        "Start": 0.004,
        "End": 0.0045000000000000005,
        "Percent": 1.10272780034823,
        "Count": 4
      },
      {
        "Start": 0.0045000000000000005,
        "End": 0.005,
        "Percent": 1.7991874637260592,
        "Count": 12
      },
      {
        "Start": 0.005,
        "End": 0.006,
        "Percent": 2.7858386535113175,
        "Count": 17
      },
      {
        "Start": 0.006,
        "End": 0.007,
        "Percent": 5.62971561230412,
        "Count": 49
      },
      {
        "Start": 0.007,
        "End": 0.008,
        "Percent": 14.103308183401044,
        "Count": 146
      },
      {
        "Start": 0.008,
        "End": 0.009000000000000001,
        "Percent": 23.447475333720256,
        "Count": 161
      },
      {
        "Start": 0.009000000000000001,
        "End": 0.01,
        "Percent": 32.84968078932095,
        "Count": 162
      },
      {
        "Start": 0.01,
        "End": 0.012,
        "Percent": 50.435287289611146,
        "Count": 303
      },
      {
        "Start": 0.012,
        "End": 0.014,
        "Percent": 65.00290191526408,
        "Count": 251
      },
      {
        "Start": 0.014,
        "End": 0.016,
        "Percent": 75.97214161346488,
        "Count": 189
      },
      {
        "Start": 0.016,
        "End": 0.018000000000000002,
        "Percent": 84.32965757399884,
        "Count": 144
      },
      {
        "Start": 0.018000000000000002,
        "End": 0.02,
        "Percent": 90.71387115496228,
        "Count": 110
      },
      {
        "Start": 0.02,
        "End": 0.025,
        "Percent": 94.6604759141033,
        "Count": 68
      },
      {
        "Start": 0.025,
        "End": 0.030000000000000002,
        "Percent": 96.57573998839234,
        "Count": 33
      },
      {
        "Start": 0.030000000000000002,
        "End": 0.035,
        "Percent": 98.3749274521184,
        "Count": 31
      },
      {
        "Start": 0.035,
        "End": 0.04,
        "Percent": 99.41961694718515,
        "Count": 18
      },
      {
        "Start": 0.04,
        "End": 0.045000000000000005,
        "Percent": 99.53569355774812,
        "Count": 2
      },
      {
        "Start": 0.045000000000000005,
        "End": 0.05,
        "Percent": 99.94196169471851,
        "Count": 7
      },
      {
        "Start": 0.05,
        "End": 0.056114808,
        "Percent": 100,
        "Count": 1
      }
    ],
    "Percentiles": [
      {
        "Percentile": 50,
        "Value": 0.01195049504950495
      },
      {
        "Percentile": 75,
        "Value": 0.01582275132275132
      },
      {
        "Percentile": 90,
        "Value": 0.019776363636363637
      },
      {
        "Percentile": 99,
        "Value": 0.03799166666666665
      },
      {
        "Percentile": 99.9,
        "Value": 0.04948357142857149
      }
    ]
  },
  "Exactly": 0,
  "RetCodes": {
    "200": 1723
  },
  "Sizes": {
    "Count": 1723,
    "Min": 1998,
    "Max": 2002,
    "Sum": 3448072,
    "Avg": 2001.2025536854323,
    "StdDev": 0.8234479988395258,
    "Data": [
      {
        "Start": 1998,
        "End": 2000,
        "Percent": 13.116656993615786,
        "Count": 226
      },
      {
        "Start": 2000,
        "End": 2002,
        "Percent": 100,
        "Count": 1497
      }
    ],
    "Percentiles": null
  },
  "HeaderSizes": {
    "Count": 1723,
    "Min": 124,
    "Max": 124,
    "Sum": 213652,
    "Avg": 124,
    "StdDev": 0,
    "Data": [
      {
        "Start": 124,
        "End": 124,
        "Percent": 100,
        "Count": 1723
      }
    ],
    "Percentiles": null
  },
  "URL": "http://localhost:8186/fortio/?labels=Fortio\u0026url=http%253A%252F%252Flocalhost%253A8080%252Fecho\u0026t=3s\u0026qps=1000\u0026save=on\u0026r=0.0001\u0026load=Start",
  "SocketCount": 9
}

Are you asking for Sizes and HeaderSizes datas?

ldemailly · 2018-03-05T21:18:52Z

no, this:

"Percentiles": null

…centile is set.

codecov · 2018-03-06T08:17:54Z

Codecov Report

Merging #179 into master will decrease coverage by 0.3%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master    #179     +/-   ##
========================================
- Coverage    90.9%   90.7%   -0.3%     
========================================
  Files          10      10             
  Lines        1643    1670     +27     
========================================
+ Hits         1494    1514     +20     
- Misses        100     104      +4     
- Partials       49      52      +3

Impacted Files	Coverage Δ
fgrpc/grpcrunner.go	`85.1% <0%> (-4.9%)`	⬇️

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 2921513...86e8429. Read the comment docs.

olimpias · 2018-03-06T17:04:02Z

The error is interesting. How does that happen

ldemailly · 2018-03-06T19:18:25Z

to test:

use the UI, change the values; say use "63, 81" and check you get p63 and p81 results
use the url above, check you get the 5 defaults
use the command line tool; check you get the 5 defaults
use the command line tool; pass -p check it works (e.g fortio load -p "63,81" www.google.com)

I doubt that the pr works given what/where you made a change ?
( circle linter spurious error is #178 )

ldemailly

see comment

olimpias · 2018-03-07T19:17:28Z

I have tested 4 case that you have mentioned, the test cases works. I think, #164 is already solved. The changes that I have done, are unnecessary. I thought that when you say "Percentiles": null, you were talking about HeaderSizes's percentile and Size's percentile.

ldemailly · 2018-03-07T19:42:10Z

the code in this pr is unrelated to what needs to be solved

one more time; with the url in #179 (comment)

what is missing is:

# target 50% 0.000371099
# target 75% 0.000461104
# target 90% 0.000540346
# target 99% 0.0007125
# target 99.9% 0.0012293

or in json:

   "Percentiles": [
      {
        "Percentile": 50,
        "Value": 0.0003710989010989011
      },
      {
        "Percentile": 75,
        "Value": 0.00046110414052697617
      },
      {
        "Percentile": 90,
        "Value": 0.0005403458213256485
      },
      {
        "Percentile": 99,
        "Value": 0.0007124999999999994
      },
      {
        "Percentile": 99.9,
        "Value": 0.0012293028000000038
      }

or on UI:

vs

olimpias · 2018-03-07T19:53:14Z

Ah I see now, I were putting URL that you shared with me to fortio UI's URL field instead of putting it to browser url. Sorry

ldemailly · 2018-03-10T23:13:39Z

ping :)

…request.

ldemailly · 2018-03-12T21:47:05Z

this is good, you just need to undo your vendor change:
https://github.com/istio/istio/wiki/Vendor-FAQ#i-did-something-and-now-my-pr-has-a-vendor-change-how-do-i-fix-it-

and I need to check how come the build doesn't check that

ldemailly · 2018-03-12T22:08:59Z

#201 fixed the issue that your pr didn't trigger a build failure when it should have, now it does

ldemailly · 2018-03-12T22:10:36Z

ui/uihandler.go

@@ -183,6 +185,9 @@ func Handler(w http.ResponseWriter, r *http.Request) {
 		log.Fatalf("expected http.ResponseWriter to be an http.Flusher")
 	}
 	out := io.Writer(os.Stderr)
+	if len(percList) == 0 {


can you make the difference between p= isn't provided, in which case we use defaults and p= is empty in which case we should respect the will of the caller to not calculate any % ?
(minor so maybe this can go in as is without that complete fix)

This reverts commit 9d0993a.

This reverts commit b22b4c6.

ldemailly · 2018-03-13T07:23:26Z

Gopkg.lock

@@ -2,58 +2,1081 @@


 [[projects]]
+  name = "cloud.google.com/go"


ahem NO please do not change Gopkg.lock (nor vendor)

ldemailly · 2018-03-13T07:26:05Z

ui/uihandler.go

@@ -183,6 +185,9 @@ func Handler(w http.ResponseWriter, r *http.Request) {
 		log.Fatalf("expected http.ResponseWriter to be an http.Flusher")
 	}
 	out := io.Writer(os.Stderr)
+	if len(percList) == 0 && !strings.Contains(r.URL.RawQuery,"p=") {


cool, that's creative and will do ! thanks!

olimpias · 2018-03-13T07:27:27Z

I mess up about vendor.

ldemailly · 2018-03-13T07:27:47Z

are you having difficulties with
https://github.com/istio/istio/wiki/Vendor-FAQ#i-did-something-and-now-my-pr-has-a-vendor-change-how-do-i-fix-it-

please tell me exactly what doesn't work so I can improve the instructions on how to fix vendor when you accidentally changed it

then we can merge this

ldemailly · 2018-03-13T07:28:29Z

please make sure to resolve "This branch is out-of-date with the base branch" first (click the button for instance, or do it on your local copy)

olimpias · 2018-03-13T07:29:57Z

When I use git submodule update. There are three diffs occurred. I was expecting that only vendor to be diff.

ldemailly · 2018-03-13T07:30:40Z

ps: replace https://github.com/istio/istio.git by https://github.com/istio/fortio.git in the instructions! the wiki is for istio but it's same setup here

…rtio into EmptyPercentilesList

ldemailly · 2018-03-13T07:33:32Z

awesome thanks!

olimpias · 2018-03-13T07:33:48Z

I think it is okay now :). Thanks

ldemailly · 2018-03-13T07:34:19Z

a test would have been great too but this is ok, I'll test it later

olimpias · 2018-03-13T07:36:27Z

I can add unit test tomorrow.

ldemailly · 2018-03-13T07:38:17Z

hmm not sure why the linters aren't happy, some whitespace issue maybe?

olimpias · 2018-03-13T07:39:13Z

Yes, while I was reverse committing. I made white space mistake.

removing extra white space

cem turker and others added 2 commits March 5, 2018 22:57

Null Percentile is fixed for Json.

427dfc5

Merge branch 'master' into EmptyPercentilesList

5b5d9eb

cem turker added 3 commits March 6, 2018 10:13

Changes reverted.

378dfa9

Percentile wont return null for Sizes and HeaderSizes header when per…

46dfb31

…centile is set.

Unneccesarry diff is removed.

c9bfece

Merge branch 'master' into EmptyPercentilesList

213e194

ldemailly requested changes Mar 7, 2018

View reviewed changes

ldemailly and others added 5 commits March 10, 2018 15:13

Merge branch 'master' into EmptyPercentilesList

a3a7df7

Default percentile is set while there is no percentile is found from …

27c7a7e

…request.

merge

498ac1b

Missed file.

f8e37d0

Merge branch 'master' into EmptyPercentilesList

a80a53d

ldemailly mentioned this pull request Mar 12, 2018

Add vendor check at the right place so it get caught by circleci #201

Merged

Merge branch 'master' into EmptyPercentilesList

c08a1b5

ldemailly reviewed Mar 12, 2018

View reviewed changes

empty p= check is added.

9d0993a

cem turker added 3 commits March 13, 2018 10:14

Vendor fix

b22b4c6

Revert "empty p= check is added."

b229498

This reverts commit 9d0993a.

Revert "Vendor fix"

eea9525

This reverts commit b22b4c6.

ldemailly reviewed Mar 13, 2018

View reviewed changes

fix

47f4390

ldemailly reviewed Mar 13, 2018

View reviewed changes

ldemailly approved these changes Mar 13, 2018

View reviewed changes

Merge branch 'master' into EmptyPercentilesList

bba22e3

cem turker added 2 commits March 13, 2018 10:32

vendor fix

7ddeb4a

Merge branch 'EmptyPercentilesList' of https://github.com/olimpias/fo…

a21542c

…rtio into EmptyPercentilesList

Merge branch 'master' into EmptyPercentilesList

436f80b

format fix

905e2a5

code review comment

86e8429

removing extra white space

ldemailly merged commit f7c66a7 into fortio:master Mar 13, 2018

olimpias deleted the EmptyPercentilesList branch March 13, 2018 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#164 Fortio remote test URL shouldn't require the percentiles list #179

#164 Fortio remote test URL shouldn't require the percentiles list #179

olimpias commented Mar 5, 2018

ldemailly commented Mar 5, 2018

olimpias commented Mar 5, 2018

ldemailly commented Mar 5, 2018

olimpias commented Mar 5, 2018 •

edited

Loading

ldemailly commented Mar 5, 2018

codecov bot commented Mar 6, 2018 •

edited

Loading

olimpias commented Mar 6, 2018 •

edited

Loading

ldemailly commented Mar 6, 2018 •

edited

Loading

ldemailly left a comment

olimpias commented Mar 7, 2018

ldemailly commented Mar 7, 2018

olimpias commented Mar 7, 2018 •

edited

Loading

ldemailly commented Mar 10, 2018

ldemailly commented Mar 12, 2018

ldemailly commented Mar 12, 2018

ldemailly Mar 12, 2018 •

edited

Loading

ldemailly Mar 13, 2018

ldemailly Mar 13, 2018

olimpias commented Mar 13, 2018

ldemailly commented Mar 13, 2018

ldemailly commented Mar 13, 2018

olimpias commented Mar 13, 2018

ldemailly commented Mar 13, 2018

ldemailly commented Mar 13, 2018

olimpias commented Mar 13, 2018

ldemailly commented Mar 13, 2018

olimpias commented Mar 13, 2018

ldemailly commented Mar 13, 2018

olimpias commented Mar 13, 2018

#164 Fortio remote test URL shouldn't require the percentiles list #179

#164 Fortio remote test URL shouldn't require the percentiles list #179

Conversation

olimpias commented Mar 5, 2018

ldemailly commented Mar 5, 2018

olimpias commented Mar 5, 2018

ldemailly commented Mar 5, 2018

olimpias commented Mar 5, 2018 • edited Loading

ldemailly commented Mar 5, 2018

codecov bot commented Mar 6, 2018 • edited Loading

Codecov Report

olimpias commented Mar 6, 2018 • edited Loading

ldemailly commented Mar 6, 2018 • edited Loading

ldemailly left a comment

Choose a reason for hiding this comment

olimpias commented Mar 7, 2018

ldemailly commented Mar 7, 2018

olimpias commented Mar 7, 2018 • edited Loading

ldemailly commented Mar 10, 2018

ldemailly commented Mar 12, 2018

ldemailly commented Mar 12, 2018

ldemailly Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

ldemailly Mar 13, 2018

Choose a reason for hiding this comment

ldemailly Mar 13, 2018

Choose a reason for hiding this comment

olimpias commented Mar 13, 2018

ldemailly commented Mar 13, 2018

ldemailly commented Mar 13, 2018

olimpias commented Mar 13, 2018

ldemailly commented Mar 13, 2018

ldemailly commented Mar 13, 2018

olimpias commented Mar 13, 2018

ldemailly commented Mar 13, 2018

olimpias commented Mar 13, 2018

ldemailly commented Mar 13, 2018

olimpias commented Mar 13, 2018

olimpias commented Mar 5, 2018 •

edited

Loading

codecov bot commented Mar 6, 2018 •

edited

Loading

olimpias commented Mar 6, 2018 •

edited

Loading

ldemailly commented Mar 6, 2018 •

edited

Loading

olimpias commented Mar 7, 2018 •

edited

Loading

ldemailly Mar 12, 2018 •

edited

Loading