yarpc / yab Goto Github PK

I noticed that when I specify a timeout of 2s, the actual header that is sent by Yab is Context-Ttl-Ms: 1999. This is because the code sets a "deadline" on the context object, and then uses that deadline for each request. Therefore any time that elapses between when the deadline is calculated and when Yab actually schedules the first request will be subtracted from the request time.

I think this is counter-intuitive. The deadline should be computed at the same time the first request is made, not before.

The error message says please specify a peer or peer list, even though one was specified, just that it was empty.

I'm getting the following after running this.

go get -u -f github.com/yarpc/yab

package github.com/yarpc/yab
    imports github.com/yarpc/yab/encoding
    imports github.com/yarpc/yab/thrift
    imports github.com/thriftrw/thriftrw-go/envelope
    imports go.uber.org/thriftrw/internal/envelope/exception: use of internal package not allowed

Two examples of this that show the correlation,

[bean@staging02 ~/yab] (master)$ ./yab-linux-amd64 moe --health -d 5s -p staging02:21300 --connections=100 --concurrency=2 --rps=100
{
  "body": {
    "result": {
      "ok": true
    }
  },
  "trace": "49a9f05c8f3f7cd4"
}

Benchmark parameters:
  CPUs:            24
  Connections:     100
  Concurrency:     2
  Max requests:    1000000
  Max duration:    5s
  Max RPS:         100
Latencies:
  0.5000: 1.138842ms
  0.9000: 1.573129ms
  0.9500: 2.976241ms
  0.9900: 13.437581ms
  0.9990: 29.176007ms
  0.9995: 31.255959ms
  1.0000: 33.335911ms
Elapsed time:      6.99s
Total requests:    500
RPS:               71.53

[bean@staging02 ~/yab] (master)$ time ./yab-linux-amd64 moe --health -d 5s -p staging02:21300 --connections=100 --concurrency=2 --rps=10
{
  "body": {
    "result": {
      "ok": true
    }
  },
  "trace": "5cbdf15a54eadb07"
}

Benchmark parameters:
  CPUs:            24
  Connections:     100
  Concurrency:     2
  Max requests:    1000000
  Max duration:    5s
  Max RPS:         10
Latencies:
  0.5000: 1.335166ms
  0.9000: 3.4625ms
  0.9500: 8.540437ms
  0.9900: 24.980317ms
  0.9990: 34.061893ms
  0.9995: 34.566425ms
  1.0000: 35.070957ms
Elapsed time:      24.9s
Total requests:    50
RPS:               2.01

real    0m25.341s
user    0m0.328s
sys     0m0.107s

Currently, yab implements a custom transport layer, but this only supports HTTP + TChannel.

If we migrate the transport layer to use YARPC, we can use all transports that YARPC supports (HTTP, TChannel, Thrift, gRPC, etc).

Currently not possible to capture in YAML template.

Right now we only take one body and duplicate it for every request within a benchmark. @ZymoticB pointed out that it's sometimes useful to benchmark different request bodies concurrently, and I wanted to open up a conversation around whether we could (or should) support something like that.

Concretely, this means something like "I want half of my benchmark's requests to use a body FOO and the other half to use BAR."

I was thinking we might be able to support a directory argument, and each file therein would be interpreted as a unique request body. With a heuristic like "append .headers to the request's filename" we could also attach per-request headers. This would leave a directory structure like so:

requests/
    foo
    foo.headers
    bar

$ yab --requests=./requests ...

This would cause yab to round-robin between foo and bar; and foo would override headers provided on the CLI.

The user could then control the distribution of requests with this file structure. For example if I want 2/3 of requests to be FOO and 1/3 to be BAR I would simply drop FOO.1 and FOO.2 into the directory.

Thoughts?

We should update all our dependencies to latest. Since we print out the trace ID when we make a request, we'll need to figure out how to make this work with the latest OpenTracing integration for TChannel.

Ideally we should not require a dependency on the Jaeger tracer.

cc @yurishkuro @kriskowal

Currently, you can't go get yab since we're using an older version of the thriftrw-go API.

See errors:

# github.com/yarpc/yab/thrift
../../../github.com/yarpc/yab/thrift/from_wire.go:75: undefined: wire.List
../../../github.com/yarpc/yab/thrift/from_wire.go:77: too many arguments in call to wire.ValueListToSlice
../../../github.com/yarpc/yab/thrift/from_wire.go:88: undefined: wire.Set
../../../github.com/yarpc/yab/thrift/from_wire.go:92: undefined: wire.List
../../../github.com/yarpc/yab/thrift/from_wire.go:95: undefined: wire.Map
../../../github.com/yarpc/yab/thrift/from_wire.go:97: too many arguments in call to wire.MapItemListToSlice
../../../github.com/yarpc/yab/thrift/to_wire.go:76: undefined: wire.List
../../../github.com/yarpc/yab/thrift/to_wire.go:79: undefined: wire.List
../../../github.com/yarpc/yab/thrift/to_wire.go:86: undefined: wire.List
../../../github.com/yarpc/yab/thrift/to_wire.go:132: undefined: wire.Map
../../../github.com/yarpc/yab/thrift/to_wire.go:86: too many errors

Now that yarpc disables the thrift envelope by default we should do the same in yab!

yab does not currently prefix application headers with Rpc-Header- over HTTP. I propose that we repurpose TOpts --topts for raw HTTP headers and alias that with -T, so -T for transport headers, -H for application headers, -B for baggage headers.

yab --rps 1 -d 5s {$QUERY_PARAMS}

Error message:

Failed to warmup connections for benchmark: tchannel error ErrCodeBusy: {SERVICE_NAME} is rate-limited by the service rps of 100

yab --rps 1 -d 5s --warmup 0 {$QUERY_PARAMS} can bypass this error.

When making HTTP requests, yab currently shows all response HTTP headers in the "headers" section. Should we be stripping common HTTP headers away (unless explicitly left on by a transport option), or should headers always be hidden?

Current output:

{
  "body": {
    "result": 5
  },
  "headers": {
    "Content-Length": "23",
    "Content-Type": "application/x-thrift",
    "Date": "Wed, 01 Jun 2016 15:55:10 GMT"
  }
}

Yab should accept an alternate peer chooser for use in conjunction with -P and benchmarks or --fanout, e.g., yab --peer-list peers --choose least-pending --fan-out.

Error saying cannot specify peer using --peer and --peer-list.

yab currently does not support TMultiplexedProtocol for HTTP/Thrift services. The only difference is that instead of using the $methodName in the envelope, it will use $serviceName$delimiter$methodName, where $delimiter is configurable and it defaults to :.

Right now, we require the service name (which is intended to be the TChannel service name), but this isn't required for HTTP

Apparently this doesn't work, @zhouzhuojie can you provide some sample input?

Currently we do not expose the TChannel "ApplicationError" bit. It's possible due to bugs that a server or relay returns the body of a Thrift exception, but does not set the application error bit correctly.

These issues will be much easier to debug if the application error is exposed on the response (similar to tcurl which returns an "ok" field)

When running benchmarks would like to have some script consumable output. JSON, CSV, etc...

When yarpc/yarpc-go#728 is resolved, we can add into yab the ability to fetch introspection details about registered handlers. This should help in debugging services.

Using all parameters i am hitting the max requests over 20 minutes with 100000000000000 max requests.

-n -1

would do just fine :D

Currently on 0.2.1 for both.

People use the --caller option to emulate tcurl's caller name, but this masks who is actually making the call. Let's prevent it.

As per yarpc/yarpc-go#238, the Rpc-Encoding HTTP header is now required.

This seems to violate POLA.

% diff -u =(yab) =(yab --help)
--- /tmp/zshqZl9Ex	2016-11-29 00:12:26.692863270 +0000
+++ /tmp/zshJIFTy8	2016-11-29 00:12:26.700863481 +0000
@@ -19,14 +19,14 @@
       --headers-file=            Path of a file containing the headers in JSON or YAML
   -B, --baggage=                 Individual context baggage header as a key:value pair per flag
       --health                   Hit the health endpoint, Meta::health
-      --timeout=                 The timeout for each request. E.g., 100ms, 0.5s, 1s. If no unit is specified, milliseconds are assumed.
+      --timeout=                 The timeout for each request. E.g., 100ms, 0.5s, 1s. If no unit is specified, milliseconds are assumed. (default: 1s)
       --disable-thrift-envelope  Disables Thrift envelopes (disabled by default for TChannel)
       --multiplexed-thrift       Enables the Thrift TMultiplexedProtocol used by services that host multiple Thrift services on a single endpoint.

 Transport Options:
   -s, --service=                 The TChannel/Hyperbahn service name
   -p, --peer=                    The host:port of the service to call
-  -P, --peer-list=               Path of a JSON or YAML file containing a list of host:ports
+  -P, --peer-list=               Path of a JSON or YAML file containing a list of host:ports (default: /path/to/hosts/file)
       --caller=                  Caller will override the default caller name (which is yab-$USER).
       --rk=                      The routing key overrides the service name traffic group for proxies.
       --rd=                      The routing delegate overrides the routing key traffic group for proxies.
@@ -35,11 +35,14 @@
   -T, --topt=                    Transport options for TChannel, protocol headers for HTTP

 Benchmark Options:
-  -n, --max-requests=            The maximum number of requests to make. 0 implies no limit.
-  -d, --max-duration=            The maximum amount of time to run the benchmark for. 0 implies no duration limit.
+  -n, --max-requests=            The maximum number of requests to make. 0 implies no limit. (default: 0)
+  -d, --max-duration=            The maximum amount of time to run the benchmark for. 0 implies no duration limit. (default: 0s)
       --cpus=                    The number of OS threads
       --connections=             The number of TCP connections to use
-      --warmup=                  The number of requests to make to warmup each connection
-      --concurrency=             The number of concurrent calls per connection
-      --rps=                     Limit on the number of requests per second. The default (0) is no limit.
+      --warmup=                  The number of requests to make to warmup each connection (default: 10)
+      --concurrency=             The number of concurrent calls per connection (default: 1)
+      --rps=                     Limit on the number of requests per second. The default (0) is no limit. (default: 0)
       --statsd=                  Optional host:port of a StatsD server to report metrics
+
+Help Options:
+  -h, --help                     Show this help message

Currently the JSON to Thrift interpreter accepts file for strings and binary blobs, to read a file. It currently uses the working directory as the base directory for that path. This should be relative to the YAML base directory when used with -y. We need to thread the base directory through the JSON to Thrift marshaller.

We often have common queries (either for testing, or administration) that have some parameters.

It would be nice if we could use a YAML template and parameterize parts of it (e.g., the service name, or some part of the request body).

E.g., if --health didn't exist, we might have a yaml template like:

#!/usr/bin/yab -y

service: $service
thrift: ./idl/meta.health
procedure: Meta::health
body:
  user: ${user:prashant}

And we could do ./health.yaml --arg service:foo --arg user:foo

We suggest yab templates use the following shebang #!/usr/bin/env yab -y, which works on Darwin, but not Linux.

cat ~/.config/yab/defaults.ini
[request]
timeout = 2s
[benchmark]
warmup = 10

$ yab [...]
Failed to parse options: error reading defaults: couldn't read /home/abeinstein/.config/yab/defaults.ini: could not find option group `request'

cc @abeinstein

Currently we just return an error:

$ yab -p 127.0.0.1:5437 moe -t /usr/share/uber-idl/code.uber.internal/rt/moe/moe.thrift
Failed while parsing input: no procedure specified, specify --procedure [procedure]

Instead we should list all available procedures

We’ve recently added support for TChannel and HTTP routing keys and routing delegates. We should expose these with flags.

For TChannel, we will need to upgrade, then use the call options for RoutingDelegate and RoutingKey.

For HTTP, the corresponding headers are Rpc-Routing-Delegate and Rpc-Reouting-Key.

This could be a hidden option. Sorry if we already support this -- it looks like it might fit into --topt but I didn't dig in.

https://github.com/yarpc/yab/blob/dev/main.go#L275

When hitting endpoints that are meant to produce errors or timeouts using benchmark with --warmup 0 yab still does not start the benchmark. It would be nice if it just retired until it hit a good request one.

A peer list is not sufficient for HTTP with TLS or an alternate path. We can use the -p flag as a template and punch addresses from the peer list into that template.

Users often don't realize that yab has a man page. Let's mention the man page in the --help output, and possibly also link to the HTML man page

It'd be useful to add an option view response headers. In addition, a header that specifies the host that handled the request would be useful information for debugging.

It's not currently possible to call Oneway procedures with yab.

Currently, the timeout in the context is used to set the timeout header, but we're not setting the context on the request, and letting Go handle timeouts for us.

Currently, warmup errors are not explicitly explained. The user also has no control over warmups currently.

Currently, if HTTP hits any error, it just returns the response code. We should include the body (if any) in the error message.

We now have standard ways to transport opentracing baggage with HTTP and TChannel. We should expose a --baggage flag that threads the appropriate headers for each transport.

We may need to also provide a way to configure the opentracing baggage prefix, both on the CLI and through yabrc.

Feature request

If -t/--thrift is passed a directory, yab will traverse it to find an appropriate *.thrift definition file for the service being called.

Motivation

The primary motivator for this change is simplicity for end-users.

In ecosystems where users make adhoc yab calls across numerous microservices, it can be cumbersome to constantly specify a *.thrift file with the -t/--thrift flag. Users will be able to specify a root IDL directory (e.g. /usr/share/my-idls/), and yab will find the right definition file for the service being called.

Further, the -t/--thrift setting could be predefined in defaults.ini to reduce the need for supplying the flag altogether.

Discussion

Namespace collisions of service names could make it difficult to find the thrift file for the intended service being invoked. This shouldn't be a problem for most usage, but it's not immediately clear how to handle those edge cases.

Additionally, there is some non-trivial overhead to traversing a large set of thrift definition files. For a tool like yab, this is possibly undesirable, but users always have the option to explicitly list a file using the -t/--thrift flag if speed becomes a concern.

Today, -p specifies a single peer, and for HTTP also happens to specify the URL template for the request including the protocol scheme of choice as well as the path. -P specifies a peer list, currently only supporting a file in YAML or JSON or flat text format. We propose to extend -P to accept a URI with multiple supported protocols including file: (to explicate the current behavior), and add http: to support grabbing the content via an HTTP request, and allow registering alternate protocols.

I think the right solution is to use the plugin support in go1.8 to load plugins that can provide custom peer providers.

type PeerProvider interface {
	Schemes() []string
	Resolve(context.Context, *url.URL) ([]string, error)
}

Resolve returns an array of alternative flags for -p, which over HTTP continues to serve as the URL template and can be used in combination with -P. In the absence of a provided -p, the selected peer template string is used both for the address and the template.

$ yab -P alt://blah/blah:http --health

When using -P, a single request will randomly select a peer, while benchmarking will round-robin between peers.

It would be nice to support some sort of a --fanout option to fanout a request to all peers. E.g.,

$ yab -p 1.1.1.1:1 -p 2.2.2.2:2 moe --health
1.1.1.1:1
[ health output]
2.2.2.2:2
[ health output ]

Benchmarking with --fanout should benchmark each individual host:port individually and return results so we can easily determine which instance is slower.

cc: @nomis52

$ yab --peer-list=hosts.json --service=autobahn --method=hosts_v1 --encoding=json --request='{"serviceName":"foo"}'
Failed while parsing response: failed to parse JSON: json: cannot unmarshal array into Go value of type map[string]interface {}

with tcurl the response is

$ tcurl -H hosts.json autobahn hosts_v1 -3 '{"serviceName":"foo"}' | jq .
{
  "ok": true,
  "head": null,
  "body": [...],
  "headers": {
    "as": "json"
  },
  "trace": "7075701ec1913375"
}