grpc support for UI #152
grpc support for UI #152
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
ui/uihandler.go
Outdated
| @@ -265,8 +275,53 @@ func Handler(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
| uiRunMapMutex.Unlock() | |||
| } | |||
| case run: | |||
| // mode == run case: | |||
| case rungrpc: | |||
There was a problem hiding this comment.
more code should be shareable between the 2 cases
see how fortio_main.go does grpc/http with sharing the interface between the 2
|
@ldemailly thank you for the pointers. I refactored the shared code and handled the default case. google cla: I signed it! |
ui/uihandler.go
Outdated
| if err != nil { | ||
| log.Errf("Init error %+v : %v", o, err) | ||
| log.Errf("Init error %+v : %v", ro, err) |
There was a problem hiding this comment.
we do lose some visibility here. but I did not think this merited creation of a HasRunnerOptions interface
There was a problem hiding this comment.
it's ok, though maybe add the input url and runner var
|
can you put just |
Codecov Report
@@ Coverage Diff @@
## master #152 +/- ##
=======================================
- Coverage 88.4% 88% -0.4%
=======================================
Files 7 7
Lines 1488 1518 +30
=======================================
+ Hits 1315 1336 +21
- Misses 115 121 +6
- Partials 58 61 +3
Continue to review full report at Codecov.
|
|
I signed it! |
|
I am trying to work out why the bot doesn't seem to pick up the corporate CLA. will ping once I have addressed that |
|
I signed it |
|
CLAs look good, thanks! |
|
I am trying to test the PR by doing a grpc run through the UI. From the UI, I change URL to However, I am able to |
|
@danehans can you try without the http prefix? i am not entirely sure if the http prefix should be honored for grpc calls? |
Webtest.sh
Outdated
| $CURL "${BASE_FORTIO}fetch/localhost:8080$FORTIO_UI_PREFIX?url=http://localhost:8080/debug&load=Start&qps=-1&json=on" | grep ActualQPS | ||
| # Check we can connect, and run a grpc QPS test against ourselves through fetch | ||
| $CURL "${BASE_FORTIO}fetch/localhost:8080$FORTIO_UI_PREFIX?url=localhost:8079&load=Start&qps=-1&json=on&runner=grpc" | grep ActualQPS |
There was a problem hiding this comment.
@danehans you raised a good point about the intended usage of grpc mode. I added an acceptance test here. let me know if this helps
There was a problem hiding this comment.
maybe grep for something GRPC specific like Health SERVING
There was a problem hiding this comment.
the grep is on the result json object which is currently shared between the http/grpc modes. the result object does not have anything grpc-specific for now.
Health SERVING is printed to stdout. not to the json file. correct me if I understood this wrong.
There was a problem hiding this comment.
k can you file a follow up that the GRPC json is missing the equivalent of the Http code map that http has
(don't need to fix everything in this 1 pr if you don't have time)
There was a problem hiding this comment.
not sure that I understand this. the grpc object does have a retcode map object eg from a sample run of 3000 queries on localhost:8079
"RetCodes": {
"1": 3000
}
however, the json doesn't contain a grpc-specific message eg Health SERVING, this is logged to stdout, hence can't be grep-ed for in the json result.
are you suggesting that the result.json should contain http/grpc specific messages?
There was a problem hiding this comment.
so the code printing stdout is
fmt.Printf("Health %s : %d\n", k.String(), total.RetCodes[k])
for some reason the Json has the numerical value "1" instead of "SERVING" - I wonder if that's fixable easily with some annotation on the struct
but we can postpone - I guess just grep for "1": 100 and use exactly 100 (-n 100)
There was a problem hiding this comment.
ah, now i get it! will give it a shot
There was a problem hiding this comment.
not sure how the automation works on this repo? will referencing the issue auto-close it if PR is merged?
|
we could be nice and strip the http/https prefix for grpc speaking of which can you add a checkbox for the Secure option of grpc ? |
|
@danehans the error is definitely obscure, so good find! |
|
added a grpc secure option not totally convinced about the automatic translation of http/https to equivalent grpc secure/insecure mode. |
|
I removed |
|
Here is a screenshot of the diagram showing http://localhost:8079"
|
|
tl:dr; fixing the title will require a larger refactoring of RunnerResults and its derived structs the value for the diagram's title is determined here the GRPCRunnerResults do not contain a URL json field, instead they contain Destination. However, Destination isn't currently written out to the result file for a grpc result save so the value for the title should show up as undefined. I am not sure how the |
|
fail fast is ok I guess - the message is obscure and delayed, I would expect the Dial to fail because http://x isn't a valid hostname |
|
understood. do you want the validation to be added as part of this PR? |
| uiRunMapMutex.Unlock() | ||
| var res periodic.HasRunnerResult | ||
| var err error | ||
| if runner == modegrpc { |
Webtest.sh
Outdated
| $CURL "${BASE_FORTIO}fetch/localhost:8080$FORTIO_UI_PREFIX?url=http://localhost:8080/debug&load=Start&qps=-1&json=on" | grep ActualQPS | ||
| # Check we can connect, and run a grpc QPS test against ourselves through fetch | ||
| $CURL "${BASE_FORTIO}fetch/localhost:8080$FORTIO_UI_PREFIX?url=localhost:8079&load=Start&qps=-1&json=on&runner=grpc" | grep ActualQPS |
There was a problem hiding this comment.
maybe grep for something GRPC specific like Health SERVING
| @@ -52,6 +52,9 @@ <h1>Φορτίο (fortio) v{{.Version}}{{if not .DoLoad}} control UI{{end}}</h1> | |||
| {{end}}{{end}} <!-- 2 extra header lines, TODO: add a JS 'more headers' button --> | |||
| <input type="text" name="H" size=40 value="" /> <br /> | |||
| <input type="text" name="H" size=40 value="" /> <br /> | |||
| http: <input type="radio" name="runner" value="http" checked/>, | |||
| grpc: <input type="radio" name="runner" value="grpc"/><br/> | |||
| Use secure transport (tls) for GRPC <input type="checkbox" name="grpc-secure"/><br/> | |||
There was a problem hiding this comment.
thanks for having used the same url param name as the flag, as is attempted for most of the other options
There was a problem hiding this comment.
yes, that was my intention. however, i did deviate a bit for the radio buttons. :|
|
the title logic is here: https://github.com/istio/fortio/blob/master/ui/static/js/fortio_chart.js#L90 edit: you already found this :-) |
|
yeah let's just add something in fgrpc/ Dial() to bail early with a nice message (though maybe just dropping http:// and https:// again would be nicer it's like when a compiler tells you "you forgot a ; on line 23" it would be nice to just assume it's there and continue with a warning instead (for compilers you can make the case it's ok to abort early to avoid doing completely wrong guesses but in this case it's pretty clear what to guess) |
fgrpc/grpcrunner.go
Outdated
| @@ -29,6 +29,8 @@ import ( | |||
| "istio.io/fortio/fnet" | |||
| "istio.io/fortio/log" | |||
| "istio.io/fortio/periodic" | |||
| "strings" | |||
| "regexp" | |||
There was a problem hiding this comment.
let’s not drag in regexp for a prefix match?
There was a problem hiding this comment.
ack. it felt like a hammer. thanks for calling it out. removed.
| { | ||
| "http hostname and port", | ||
| "http://localhost:1234", | ||
| "localhost:1234", |
| { | ||
| "https hostname and port", | ||
| "https://localhost:1234", | ||
| "localhost:1234", |
There was a problem hiding this comment.
can we check grpc secure state ?
There was a problem hiding this comment.
that is not being set in the parseGRPCDestination function. It's tied in with the fgrpc.Dial function.
if tls || (strings.HasPrefix(serverAddr, "https://")) {
opts = grpc.WithTransportCredentials(credentials.NewTLS(nil))
}
not sure how to best test it.
There was a problem hiding this comment.
you can test it (at least manually, and probably also in Webtest.sh, with
fortio grpcping https://fortio.istio.io/
(it will need to set the port to 443 too ideally if not supported)
maybe we should just bail if it has http[s]:// prefix
There was a problem hiding this comment.
sorry, was unclear earlier. i had tested it manually and the https state led to secureTLS internal state. In any case, I added an additional grpcping in webtest.sh
fgrpc/grpcrunner.go
Outdated
| @@ -156,9 +160,16 @@ func RunGRPCTest(o *GRPCRunnerOptions) (*GRPCRunnerResults, error) { | |||
| return &total, nil | |||
| } | |||
|
|
|||
| // GRPCDestination parses dest and returns dest:port based on dest type | |||
| // parseGRPCDestination parses dest and returns dest:port based on dest type | |||
There was a problem hiding this comment.
why the name/visibility change ?
There was a problem hiding this comment.
since its not being used outside the package anymore and its being embedded into the Dial method
There was a problem hiding this comment.
go style would probably be to call this grpcDestination() then but it's ok
There was a problem hiding this comment.
changed to grpcDestination
|
@ldemailly let me know if this needs any more work. rebased with current master |
Webtest.sh
Outdated
| $CURL "${BASE_FORTIO}fetch/localhost:8080$FORTIO_UI_PREFIX?url=http://localhost:8080/debug&load=Start&qps=-1&json=on" | grep ActualQPS | ||
| # Check we can connect, and run a grpc QPS test against ourselves through fetch | ||
| $CURL "${BASE_FORTIO}fetch/localhost:8080$FORTIO_UI_PREFIX?url=localhost:8079&load=Start&qps=-1&json=on&runner=grpc" | grep ActualQPS |
There was a problem hiding this comment.
so the code printing stdout is
fmt.Printf("Health %s : %d\n", k.String(), total.RetCodes[k])
for some reason the Json has the numerical value "1" instead of "SERVING" - I wonder if that's fixable easily with some annotation on the struct
but we can postpone - I guess just grep for "1": 100 and use exactly 100 (-n 100)
| docker exec fortio_server /usr/local/bin/fortio load -stdclient -qps 1 -t 2s -c 1 https://www.google.com/ | ||
| # and with normal and with custom headers | ||
| docker exec fortio_server /usr/local/bin/fortio load -H Foo:Bar -H Blah:Blah -qps 1 -t 2s -c 2 http://www.google.com/ | ||
| # Do a grpcping | ||
| docker exec fortio_server /usr/local/bin/fortio grpcping localhost | ||
| # Do a grpcping to a scheme-prefixed destination | ||
| docker exec fortio_server /usr/local/bin/fortio grpcping https://fortio.istio.io:443 |
There was a problem hiding this comment.
👍 can we make it work without :443 :-) ? [it's ok as, we can do that later]
There was a problem hiding this comment.
you mean, be intelligent and assume 443 on a https prefix?
There was a problem hiding this comment.
yes if no :port is specified but http:// prefix is there, use 80, if https:// is there, use 443
but we can do that in followup
There was a problem hiding this comment.
opened 162 to track that
fgrpc/grpcrunner.go
Outdated
| // strip any unintentional http/https scheme prefixes from destination | ||
| if strings.HasPrefix(dest, "http://") || strings.HasPrefix(dest, "https://") { | ||
| dest = strings.Replace(strings.Replace(dest, "https://", "", 1), "http://", "", 1) | ||
| log.Warnf("stripping http/https scheme. grpc destination: %v", dest) |
There was a problem hiding this comment.
it's a feature so probably Infof or LogVf
fgrpc/grpcrunner.go
Outdated
| func grpcDestination(dest string) string { | ||
| // strip any unintentional http/https scheme prefixes from destination | ||
| if strings.HasPrefix(dest, "http://") || strings.HasPrefix(dest, "https://") { | ||
| dest = strings.Replace(strings.Replace(dest, "https://", "", 1), "http://", "", 1) |
There was a problem hiding this comment.
this is a bit obscure, split the if ?
ui/uihandler.go
Outdated
| if err != nil { | ||
| log.Errf("Init error %+v : %v", o, err) | ||
| log.Errf("Init error %+v : %v", ro, err) |
There was a problem hiding this comment.
it's ok, though maybe add the input url and runner var
| if err != nil { | ||
| log.Errf("Init error %+v : %v", o, err) | ||
| log.Errf("Init error for %s mode with url %s and options %+v : %v", runner, url, ro, err) |
There was a problem hiding this comment.
enhanced the error log message
|
pro tip: please never force push a pr once reviewing has started do a pull instead of a merge/rebase to be in sync |
| "https hostname and port", | ||
| "https://localhost:1234", | ||
| "localhost:1234", | ||
| }, | ||
| { | ||
| "IPv4 address", | ||
| "1.2.3.4", |
There was a problem hiding this comment.
can you add tests for http:// and https://foo.bar:2567 -> foo.bar:2567 ?
|
sorry about the inconvenience with the force pushes. i guess i went overboard with trying to keep commit history clean |
|
it gets squashed at the end on master so the PR can have 30 commits it's no problems |
@ldemailly can you review? w.r.t #146
thanks a lot for creating this tool!
it has proven quite useful in quick evaluation of grpc services.