Enhance verbose output of conformance client and enable stream conformance tests #212
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pkwarren
approved these changes
Jan 30, 2024
| if (intVal == null || intVal < 1 || intVal > 5) { | ||
| throw RuntimeException("value for $arg option should be an integer between 1 and 5; instead got '$v'") | ||
| } | ||
| verbosity = intVal |
There was a problem hiding this comment.
I'd be in favor of just having -v <value>/-v (not -vv) or just -v <value> for simplicity.
| inetSocketAddress: InetSocketAddress, | ||
| proxy: Proxy, | ||
| protocol: Protocol?, | ||
| ioe: IOException, |
There was a problem hiding this comment.
Does it make sense to print the exception stack trace too?
There was a problem hiding this comment.
The reason I didn't is because it wasn't really useful. These errors propagate to the caller, which means these exceptions already get logged elsewhere at just verbosity 1 or 2.
jhump
force-pushed
the
jh/minor-cleanup
branch
from
43a9fa2
to
b32f62a
Compare
jhump
force-pushed
the
jh/enable-stream-conformance-tests
branch
from
c8e4b1c
to
7fad0d7
Compare
…ing okhttp event logger and logging decorator for http client
…w/ config instead of known-failing
jhump
force-pushed
the
jh/enable-stream-conformance-tests
branch
from
7fad0d7
to
e416c20
Compare
…f 499) in tracing
pkwarren
approved these changes
Jan 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds verbose output features to the conformance client. There are now 5 levels of verbosity --
-v,-vv, and-vvvcan be used to turn on the first three;-v 4and-v 5can be used to turn on the heavier trace output.I used this output, along with a newly added
--traceflag to the conformance test runner, to troubleshoot the streaming failures. (This PR, #210, and #211 represent all of the changes made to get everything to work.)The levels of verbosity are:
send. (These could be expected errors, for tests of error conditions, which is why they are not enabled at verbosity 1).HTTPClientInterfaceimplementation, to print out each step in an RPC.HTTPClientInterfacetracing, so that each step shows its full stack trace (useful since some operations can happen from different threads).This PR also applies some clean-up to the conformance configs and ... 🥁 ... enables all of the conformance tests! They now should all pass.